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

global update #9

Merged
merged 6 commits into from
Mar 18, 2016
Merged

global update #9

merged 6 commits into from
Mar 18, 2016

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Mar 18, 2016

Didn't see this code before, mostly this PR apply standard

@@ -0,0 +1,17 @@
# browserify-rsa
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we renaming the readme ? it was lowercase to make completion from the comand line easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never thought about it, but for node packages usually README.md uses, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tendency is to make the special files allcaps, but they are technically case insensitive due to windows file system being case insensitive and most mac file systems being mostly case insensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's bad that file systems are insensitive, do you really want to keep lowercase?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh since license is all caps as well, we should probably make this one all caps as well

r = new bn(randomBytes(len));

function getr (priv) {
for (var r, len = priv.modulus.byteLength(); ;) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change this to a for loop, if we are using like a while loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think while will be more readable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this context yes I believe a while loop is much more readable then the equivalent for loop considering the for loop is acting like the while loop by having an empty 3 argument

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's about do-while?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@calvinmetcalf
Copy link
Contributor

thanks for cleaning this up, I have a few nits and questions but it looks overall helpful

@fanatid fanatid force-pushed the feature/master branch 2 times, most recently from 14e970f to 23161ef Compare March 18, 2016 14:08

function getr (priv) {
var len = priv.modulus.byteLength()
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to do a

do {} while() loop ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted use do-while here, but current looks better than:

  var len = priv.modulus.byteLength()
  var r
  do {
    r = new BN(randomBytes(len))
  } while (r.cmp(priv.modulus) >= 0 || !r.umod(priv.prime1) || !r.umod(priv.prime2))
  return r

agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't the equivalent be

  var len = priv.modulus.byteLength()
  do {
    var r = new BN(randomBytes(len))
  } while (r.cmp(priv.modulus) >= 0 || !r.umod(priv.prime1) || !r.umod(priv.prime2))
  return r

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, it will work, but I don't like idea return variable that was defined in nested block scope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that's what your code does currently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I mean return variable from current scope that was defined in nested scope
right now variable defined and returned from one scope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me (personal preference):

not ok:

function f () {
 if (1) { var x = 2 }
 return x
}

ok:

function f () {
 if (1) { var x = 1; return x }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I was mis reading the change, I thought you were still returning r, anywho I don't think the while true is much of an improvement, we should probably either ditch this change for now and not include it in this particular pull.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, updated to old code
can you make change without pull after merge?

@calvinmetcalf calvinmetcalf merged commit 6c2c290 into browserify:master Mar 18, 2016
@fanatid fanatid deleted the feature/master branch March 18, 2016 17:33
@fanatid
Copy link
Contributor Author

fanatid commented Mar 18, 2016

@calvinmetcalf Thank you!
Any plans about publishing changes? At least new code uses toArrayLike from bn.js.

@fanatid fanatid mentioned this pull request Mar 19, 2016
@dcousens
Copy link
Member

@calvinmetcalf in a .inputrc:

# tab complete should ignore case
set completion-ignore-case on
set completion-map-case on

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

Successfully merging this pull request may close these issues.

3 participants