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

Fix large number decoding for unsigned types in JSON #338

Merged
merged 1 commit into from Oct 17, 2022

Conversation

jotaen
Copy link
Contributor

@jotaen jotaen commented Oct 17, 2022

I noticed that after merging the fix for large numbers (#334), the build was broken on the master branch due to a linter complaint.

Problem is that the uintDecoder is no longer a code duplication of intDecoder, because the adjustments by #334 only changed intDecoder. It’s not the linter directive that’s obsolete, but I rather think that uintDecoder must have that fix as well, because it suffers from the same problem.

In addition to the actual fix, I slightly adjusted the tests:

  • I converted the Github link to an inlined explanation, just for convenience.
  • Both cases for int and uint are now covered. I think it makes sense to have them together in one single test, since they are so closely related. I enclosed each one in a block, so that they have their own scope.
  • For the sake of completeness/consistency, I added another test for floats, which covers the floatDecoder.
  • Instead of having a helper variable n in the test, I think it’s more robust to inline the number. That way, we don’t rely on how fmt.Sprintf would serialise the number within the test.
  • Writing n := 100000 would infer the type to int, but int is platform-dependent. It’s safer to assign the type explicitly (e.g. int64), to make the test deterministic.

One note aside: the tests don’t seem to be run on PRs. Maybe that could be fixed, which would uncover build failures before they go into the master branch.

@jotaen
Copy link
Contributor Author

jotaen commented Oct 17, 2022

One note aside: the tests don’t seem to be run on PRs. Maybe that could be fixed, which would uncover build failures before they go into the master branch.

I just saw that is only an issue for first-time contributors. It might be still possible to improve the situation by marking the workflow run as required, which I think will prevent merging without running the workflow.

@alecthomas
Copy link
Owner

The tests do run, they just require approval first. This is a GitHub security measure.

Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Thanks!

@alecthomas alecthomas merged commit 32e8ffc into alecthomas:master Oct 17, 2022
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