Skip to content
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

modn API inconsistency #112

Closed
axic opened this issue Jan 25, 2016 · 9 comments
Closed

modn API inconsistency #112

axic opened this issue Jan 25, 2016 · 9 comments

Comments

@axic
Copy link
Contributor

axic commented Jan 25, 2016

All mod/div methods return a BN instance, except modn which returns a JS Number.

I think it would make sense making the current modn internal (prefixing it with _) so that toString can keep using it the fastest way, and creating a new modn which returns a BN instance.

divmod already encapsulates a modn result as a BN instance so it should use the new method.

Comments?

@axic axic changed the title modn inconsistency modn API inconsistency Jan 25, 2016
@indutny
Copy link
Owner

indutny commented Jan 27, 2016

I'm fine with fixing it, but let's postpone it until next major release.

@axic
Copy link
Contributor Author

axic commented Jan 29, 2016

When do you plan a new major release? Should this live in a branch until then? Or shouldn't we have a branch called v5.x so changes like that can be merged in there?

@indutny
Copy link
Owner

indutny commented Jan 29, 2016

Hm... I'm not sure about it yet. Do you think there is some urgency in handling this particular issue? Or do you have any other plans in mind?

@axic
Copy link
Contributor Author

axic commented Feb 1, 2016

Don't know, there could be a couple of things made more clear, etc.

@jprichardson
Copy link
Collaborator

According to https://github.com/indutny/bn.js#postfixes, any method ending with n returns a JavaScript number. Why change that here? Wouldn't that be confusing? ping @calvinmetcalf

@indutny
Copy link
Owner

indutny commented Feb 22, 2016

@jprichardson this is not exactly true. n applies only to argument, it does not say anything about return value. For example .iaddn returns BN.

@jprichardson
Copy link
Collaborator

n - which means that the argument of the function must be a plain JavaScript number

@indutny Gahhh. Haha, it says "argument" right there - sorry I missed that.

@axic
Copy link
Contributor Author

axic commented Feb 29, 2016

All the arithmetic methods return a BN except modn. Perhaps it could be a good idea to agree on a prefix or suffix for those which don't and have that as a counterpart for modn after the breaking change?

@indutny
Copy link
Owner

indutny commented Mar 1, 2016

I can't really think about anything other than rn? Like returns number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants