-
Notifications
You must be signed in to change notification settings - Fork 35
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 sn to int parsing to use base 16 #216
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## development #216 +/- ##
===============================================
- Coverage 82.86% 82.85% -0.01%
===============================================
Files 47 47
Lines 4190 4189 -1
Branches 1044 1043 -1
===============================================
- Hits 3472 3471 -1
Misses 689 689
Partials 29 29 ☔ View full report in Codecov by Sentry. |
I'm thinking this |
@@ -40,7 +40,7 @@ export class CesrNumber extends Matter { | |||
// make huge version of code | |||
code = code = NumDex.Huge; | |||
} else { | |||
throw new Error('Invalid num = {num}, too large to encode.'); | |||
throw new Error(`Invalid num = ${num}, too large to encode.`); |
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.
Good catch!
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.
Thanks for the new tests
The sequence number is expected to be a hex string without the preceeding
0x
. So for example sequence number 10 is simplya
. This causes issues with previous implementation becauseNumber("a") => NaN
this fixes the issue by specifying the base when parsing.Another potential fix could have been to add a
0x
prefix server side... But that probably has more implications that I am not aware of.It would be good to add some unit tests that verifies the behaviour with a sequence number > 10.
There is a related issue where I wrote about this a few months ago: #138. Though I think the issue is broader than this particular fix. I simply searched the codebase for usage of the
Number
constructor and replaced them where it looked like it needed replacing.