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

Support 64-bit ints encoded as json strings #12

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

davcamer
Copy link
Contributor

The proto3 standard says that both 32-bit and 64-bit integers should be
accepted as json strings. When they are emitted the 32-bit ones should
be json numbers, and the 64-bit json strings.
https://developers.google.com/protocol-buffers/docs/proto3#json

That's what I've aimed to do here. The wrapped int types should follow
the same rules.

The proto3 standard says that both 32-bit and 64-bit integers should be
accepted as json strings. When they are emitted the 32-bit ones should
be json numbers, and the 64-bit json strings.
https://developers.google.com/protocol-buffers/docs/proto3#json

That's what I've aimed to do here. The wrapped int types should follow
the same rules.
{-| Decodes an IntValue.
-}
intValueDecoder : JD.Decoder Int
intValueDecoder =
JD.int
intDecoder


{-| Encodes an IntValue.
Copy link
Owner

Choose a reason for hiding this comment

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

IIRC this is currently used for both 32 and 64 bit wrapper types, ideally we should do the same thing as for the basic types, encoding 32 bit as numbers and 64 bit as strings. WDYT?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah just noticed your comment in the conversation above mentioning this :) I'll go ahead and merge your current PR, let me know if you can also look into that, if not I will do it at some point. Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't test this as extensively, but I think this functionality is already there. I was worried how clear it was, and I guess it isn't that clear.

I believe these changes in the go code will mean that wrapped 64 bit Ints will be emitted as strings: https://github.com/tiziano88/elm-protobuf/pull/12/files#diff-346e08d50aa913dadc9d982df75f248aL56

But, it is not symmetric between the 64 and 32 bit wrapped types. The 64 bit types have their encoders generated directly to numericStringEncoder but the 32 bit types go indirectly through intValueEncoder. I could tidy that up. Or, maybe I've missed something and the 64 bit wrapped types are broken at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tiziano88 tiziano88 merged commit 8f23582 into tiziano88:master Apr 11, 2018
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.

None yet

2 participants