Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OTP27 support; Fix handling of negative zero in OTP27+ #370

Merged
merged 7 commits into from
May 18, 2024

Conversation

v0idpwn
Copy link
Collaborator

@v0idpwn v0idpwn commented May 10, 2024

Adds CI and fixes tests that fail on OTP27.
Fixes dependency (ssl_verify_fun) that would cause CI to fail on Elixir 1.15+.
Fixes warning that would cause CI to fail on OTP25 + Elixir 1.16 (closing #369).

Took the opportunity to fix negative zero handling:

According to the official documentation in
https://protobuf.dev/programming-guides/proto3/#default as of 09/05/24,
if a float or double value is set to +0 it will not be serialized, but
if it is set to -0 it should be serialized.

Currently there are no conformance tests for this behaviour.

v0idpwn added 2 commits May 9, 2024 20:51
According to the official documentation in
https://protobuf.dev/programming-guides/proto3/#default as of 09/05/24,
if a float or double value is set to +0 it will not be serialized, but
if it is set to -0 it should be serialized.

Currently there are no conformance tests for this behaviour.
Comment on lines +180 to +181
# Varies with erlang version
assert TestMsg.EnumFoo.__reverse_mapping__()[4] in [:C, :D, :E]
Copy link
Collaborator Author

@v0idpwn v0idpwn May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enum is aliased, hence it's correct to decode to any of the aliases, according to documentation:

Though all alias values are valid during deserialization, the first value is always used when serializing.

(https://protobuf.dev/programming-guides/proto3/)

The result is deterministic, just varies by erlang version.

Comment on lines +344 to +345
data = %{"j" => 1}
decoded = %Foo{j: :A}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I just avoided using an aliased enum

test/protobuf/encoder_test.exs Outdated Show resolved Hide resolved
test/protobuf/json/encode_test.exs Outdated Show resolved Hide resolved
@v0idpwn
Copy link
Collaborator Author

v0idpwn commented May 13, 2024

Looks like erleft/setup-beam is too strict and doesn't allow me to use Elixir 1.16.2 with the rc OTP...

@whatyouhide
Copy link
Collaborator

Yeah we'll need to wait for OTP 27 to be released. Should be real soon.

@v0idpwn
Copy link
Collaborator Author

v0idpwn commented May 14, 2024

Should we maybe drop the CI part (can add it in another PR), or just hold this until its released? :)

@whatyouhide
Copy link
Collaborator

@v0idpwn can you remove the CI part so that we can merge this, and open an issue to remind us to bump CI when 27 is released?

v0idpwn and others added 4 commits May 15, 2024 07:49
Required for Elixir 1.15+
They are related to ordering changes
Co-authored-by: Andrea Leopardi <[email protected]>
@v0idpwn v0idpwn force-pushed the fix/positive-negative-0 branch from a042d98 to 13d0b25 Compare May 15, 2024 10:56
@v0idpwn
Copy link
Collaborator Author

v0idpwn commented May 15, 2024

Rebased and changed to add Elixir 1.16.2 with OTP 25 instead of with OTP 27.

@ericmj
Copy link
Collaborator

ericmj commented May 17, 2024

There's a CI failure, other than it looks good.

@whatyouhide whatyouhide merged commit e122483 into elixir-protobuf:main May 18, 2024
9 checks passed
@whatyouhide
Copy link
Collaborator

Thanks @v0idpwn 🙏

@zhihuizhang17
Copy link

@whatyouhide, request a new version release in hex.pm pls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants