-
Notifications
You must be signed in to change notification settings - Fork 409
chore: make integers errors more explicit #1314
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
base: main
Are you sure you want to change the base?
Conversation
For easy debugging.
packages/math/src/integers.ts
Outdated
| public static fromString(str: string): Uint32 { | ||
| if (!str.match(/^[0-9]+$/)) { | ||
| throw new Error("Invalid string format"); | ||
| throw new Error("Invalid string format. Found: " + str); |
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.
Can you put the values in quotes? This makes it easier to find begin and end in the error messages and allows debugging empty string inputs.
Probably easier to write with a template string.
throw new Error(`Invalid string format. Found: '${str}'`);Thanks
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.
I had also thought about it, but I based it on
cosmjs/packages/math/src/integers.ts
Line 204 in 2c3b27e
| throw new Error("Invalid value in byte. Found: " + bytes[i]); |
I will adapt to reflect your suggestion.
packages/math/src/integers.ts
Outdated
| private constructor(data: BN) { | ||
| if (data.isNeg()) { | ||
| throw new Error("Input is negative"); | ||
| throw new Error("Input is negative. Found: " + data); |
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.
Does that give you a proper debug representation of a BN?
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.
I don't think so, I will adapt this with the call toString()
webmaster128
left a comment
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.
LGTM overall
|
Should be good @webmaster128 |
|
any news @webmaster128? |
|
The integer classes recently got rewritten to use |
For easy debugging.