-
Notifications
You must be signed in to change notification settings - Fork 171
encoder: Encode instances of Number as numbers, not maps #271
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
Conversation
codecov-commenter
commented
Mar 8, 2025
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@ ## main #271 +/- ## ========================================== - Coverage 97.28% 97.19% -0.09% ========================================== Files 17 17 Lines 1140 1142 +2 Branches 253 254 +1 ========================================== + Hits 1109 1110 +1 - Misses 31 32 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Why do you use the Number wrapper object? There's no reason to use it and it seems just a bug, in your project.
I am not going to go too much into the details of my project because this is not the place, but it turns out that I have a need for representing a integer using an object instance rather than a primitive type, a need which Number was built into the language to fulfill. This is a deliberate choice, not a bug.
My goal with this PR is merely to point out that Number does exist today as a core part of the language (has since ES1, really), and so the question this poses here is:
- Do we consider the MessagePack representation of it should be a map
- Or do we consider it should be a int/float?
@gfx Any thoughts on the above?
Uh oh!
There was an error while loading. Please reload this page.
Make sure instances of
Numberas encoded as anumberinstead of amap.I am encountering this issue in my current project, I believe this is a more sensible default behavior
All tests pass.