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

map: invalid keys sort #15

Closed
wI2L opened this issue Dec 6, 2019 · 4 comments · Fixed by #22
Closed

map: invalid keys sort #15

wI2L opened this issue Dec 6, 2019 · 4 comments · Fixed by #22
Assignees
Labels
bug Something isn't working

Comments

@wI2L
Copy link

wI2L commented Dec 6, 2019

@achille-roussel Hello,

As I was reading the code of the package, I noticed the sortKeys function that is created based on the map key type to encode, and saw that you sort on the reflect.Value value rather than the string representation of the key.

I was perplex to find that as an optin functionality, since the package aims to be compliant with the standard library, unless I misunderstood the README.

The following example shows that the output is different when using a map with keys of type int: https://play.golang.org/p/1ddbUssdbaf

The only reference to a map with keys of signed/unsigned-integer kind I could find in the tests is at

map[int]bool{1: false, 42: true},
but the keys are already lexicographicaly sorted.

@achille-roussel
Copy link
Contributor

@wI2L Thanks for reporting the issue! And you're correct, this seems like a mistake.

Would you be available to submit a fix? Otherwise I'll take a look at it sometimes this weekend.

@achille-roussel achille-roussel added the bug Something isn't working label Dec 6, 2019
@achille-roussel achille-roussel self-assigned this Dec 6, 2019
@wI2L
Copy link
Author

wI2L commented Dec 6, 2019

@achille-roussel Sure, I'll send a PR tomorrow, this is a quickfix.

@ofek
Copy link

ofek commented Dec 10, 2019

Any update?

@achille-roussel
Copy link
Contributor

I'm working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants