-
Notifications
You must be signed in to change notification settings - Fork 151
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
Padding fix #199
base: master
Are you sure you want to change the base?
Padding fix #199
Conversation
I think that this will make things more complex
|
Hey @fanatid thanks for the quick response! Hmm after inspecting the code a little bit more, it seems that this is indeed the intended use of the padding param. If you look at the tests at line 16, a hex number is created with a length of 7: var a = new BN('ffb9602', 16); The length of hex numbers must always be even so they use a padding of assert.equal(a.toString('hex', 2).length, 8); So this doesn't seem to be a bug but it is still rather unusual, especially for decimal numbers. Then again, I think that having two different behaviors for hex numbers and decimal numbers would probably add a lot of confusion for the user. And this would add a breaking change to the library. So I suggest having a new function explicitly made for "normal" padding. |
I really should get cracking on #36 |
I'm confused, isn't padding only applied on display? If so, why wouldn't it be applied differently for different representations? If I ask for padding when displaying a decimal, I'm going to expect something different from when I'm asking for padding for a hexadecimal. |
@therightstuff I totally agree with you that the display should behave differently depending on the base of the number. However, I think that the padding should be applied uniformly. If I call But right now, It is totally fine and not a bug, but a little bit unexpected as a user perspective. So this is why I propose a new function:
What do you think if I close this pull request and we continue the conversation here? |
Padding is typically dealt with at a byte level, the issue of toString doing any padding at all is confusing. |
@dcousens what do you mean by padding? I think it's fair to say that the majority of developers using something like bn.js will understand padding as being for representation, and when we talk about padding we talk about adding leading zeroes or blanks to fill a minimum space. If there's another possible meaning then I would say that that needs to be made clear with different method or parameter names. If toString() allows a padding parameter that is non-standard (regardless of what's "correct" or "incorrect"), then it should have a different name (not "padding", perhaps "bytePadding" or something) or be documented clearly. If we're just talking about different padding behaviour or different mechanisms between different bases then that should be transparent to the developer calling toString. |
@macor161 I agree with not introducing breaking changes, perhaps using a new signature would help? |
Fixes #198
toString
padding should only apply if length of number is smaller than requested padding.