Skip to content

Conversation

@dcousens
Copy link
Contributor

Simple enough PR, use https://github.com/feross/standard for formatting and styling.

The changes were minimal, with manual intervention over standard --format needed mostly around the mocha globals and extra if statement padding.

@dcousens
Copy link
Contributor Author

The PR is much smaller when you consider ?w=0

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) to 97.77% when pulling c6eb6a8 on standard into 376c653 on master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toString invokes affineX in a way that doesn't annoy standard

@dcousens dcousens added this to the 2.0.0 milestone Feb 22, 2015
@dcousens
Copy link
Contributor Author

@jprichardson thoughts?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) to 97.77% when pulling 6f72c4e on standard into 376c653 on master.

@dcousens
Copy link
Contributor Author

Seems @coveralls wasn't showing us the coverage loss in using the one-liner if statements.
Most of the assert branches would not be being covered fully either, perhaps we should use Exception after all...

@dcousens dcousens force-pushed the standard branch 2 times, most recently from 27b10ea to 014e2ca Compare February 24, 2015 01:45
@dcousens
Copy link
Contributor Author

Rebased on master. @kyledrake, @weilu thoughts appreciated. Hoping to merge by Wednesday 25th.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) to 97.77% when pulling 014e2ca on standard into 09d8e44 on master.

@jprichardson
Copy link
Member

@jprichardson thoughts?

Thoughts on Standard? Ya, it's great. Assuming you're talking about toString() and affineX... I think I'm missing context / understanding...

@dcousens
Copy link
Contributor Author

@jprichardson no no, I was just annotating why there was an actual code change there. I was just after your thoughts on standard.

Happy to discuss any concerns post merge.

dcousens added a commit that referenced this pull request Feb 25, 2015
@dcousens dcousens merged commit 7513e73 into master Feb 25, 2015
@dcousens dcousens deleted the standard branch February 25, 2015 03:46
Copy link
Contributor

Choose a reason for hiding this comment

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

wat? why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Standard-format artifact. @maxogden?
On 1 Mar 2015 3:11 pm, "Wei Lu" notifications@github.com wrote:

In src/ecdsa.js
#364 (comment)
:

@@ -146,7 +146,7 @@ function verifyRaw(curve, e, signature, Q) {
if (r.signum() <= 0 || r.compareTo(n) >= 0) return false
if (s.signum() <= 0 || s.compareTo(n) >= 0) return false

  • // c = s^-1 mod n
  •  // c = s^-1 mod n
    

wat? why?


Reply to this email directly or view it on GitHub
https://github.com/bitcoinjs/bitcoinjs-lib/pull/364/files#r25563881.

dcousens added a commit that referenced this pull request Mar 2, 2015
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxogden, another example of this weird comment indentation artifact

@dcousens
Copy link
Contributor Author

dcousens commented Mar 2, 2015

@weilu see 8aa4f9e and 0bba215 for a few fixes of these issues.

Hopefully @maxogden can get around to fixing a few of the oddities so we can run the tool over the code base without issue.

@weilu
Copy link
Contributor

weilu commented Mar 2, 2015

👍

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.

5 participants