-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 #15413: support JSON in int64.high+1..uint64.high as JUint, and beyond as JNumber #15545
Conversation
1f9a2a1
to
52aef1a
Compare
How does deprecation work? |
EDIT:
|
d608fe8
to
72c3539
Compare
72c3539
to
d32abd1
Compare
PTAL, green now, see above comments in #15545 (comment) for how to handle backward+forward compatibility |
This reverts commit 67363a2.
This reverts commit 67363a2.
Please consider carefully before adding any new JSON kind! If you regret later, this will be a breaking change. This cannot be undone and the interface will ruin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break code, we shouldn't merge it until it there is a transition path.
I've suggested #15646 as a way to avoid breaking changes (instead a warning would be issued until clients upgrade their code; in particular if this PR was modified to apply to individual enum members instead of the whole enum) but this was closed, and I've also suggested affected code be patched with simply adding a trailing else which is forward and backward compatible, ie after the trailing else is added to affected code it will work with both new and old enum. What is your alternative suggestion to fix #15413 and make nim parse valid json strings involving numbers (eg uint64 that don't fit int64, or beyond uint64/int64/float64) that also doesn't break any code (nor introduce ambiguities as suggested in #15413 (comment) [1])? [1] which can't distinguish string from unparsable numbers and makes certain tasks difficult in presence of those unparseable numbers, eg pretty-printing a json string, filter a json string or more generally any json=>process=>json program |
It can do that... Not that it's a big deal anyway. |
fix #15413
see rationale here: #15413 (comment), in particular wrt BigInt vs uint64
the only breakage caused by this change was in a single package,
nimpy.nim(821, 3) Error: not all cases are covered; missing: {JUInt, JNumber}
and I've already fixed that in yglukhov/nimpy#177 usingtrailing else
.(this could've also been fixed by #15646, after which I'd add
{.allowMissingCases.}
pragma as described in #15646)