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

Serialize/deserialize maps having numerical keys. Fix #125 #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ancane
Copy link
Contributor

@ancane ancane commented Dec 2, 2016

Hi. To me, having numerical keys is handy at times, and having to define custom format every time is cumbersome. So I'd like to suggest to put it here. Thanks.
This is my original work.

@ktoso
Copy link
Member

ktoso commented Dec 2, 2016

Refs #125

@lniskanen
Copy link

I had to merge this into an unmanaged jar, but hopefully this gets into release as well. Thanks for the fix.

@ancane
Copy link
Contributor Author

ancane commented Mar 15, 2017

np, thank you.

@eugeniyk
Copy link

So guys, when it's gonna be merged?

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

Hmm, seems weird to me that the signature implies that any kind of keys would be supported when in fact it has stricter constraints on the key format.

Here are two potential alternatives:

  • model key entries as { "key": ..., "value": ... } which would really support any kind of keys and values
  • create another type-class to model "Keyable" conversions, which would probably mean providing a bidirectional conversion K <=> String

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Marking as not ready to merge yet; has review feedback to be addressed

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

5 participants