Skip to content

Conversation

@dcousens
Copy link
Contributor

Just put the src/ directory through a basic first pass in JSHint.
Have mostly focused on unused variables, variable re-declarations and invalid line breaks which could have been causing unknown errors.

I don't know if Wallet.mkSend even works, simply because the code before this was using the Bitcoin. namespace, which doesn't even exist now?

Whole area is in desperate need of tests.

edit: Understandable [if this is accepted] that you may want these commits squashed.

@dcousens dcousens changed the title First pass through JSLint First pass through JSHint Mar 23, 2014
@dcousens dcousens changed the title First pass through JSHint Basic pass through JSHint Mar 23, 2014
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for commenting this out?

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 was unused.
I figured it might be a bit controversial given its proximity to the ECDSA code, therefore I didn't remove the line entirely.

@weilu
Copy link
Contributor

weilu commented Mar 23, 2014

A squashed commit would be great! And this definitely cleans things up a bit.

As I mentioned in my email to you, I'm currently working on wallet.js so this will create unfortunate conflict with the wallet branch I'm working on:https://github.com/bitcoinjs/bitcoinjs-lib/tree/wallet. Mind if we remove the changes to wallet.js and its tests for now? And once I get the wallet branch in shape (in a day or two, by my estimate), you are certainly welcome to run lint again and submit another PR.

@dcousens
Copy link
Contributor Author

For sure, I figured the biggest problem with this commit was how much it touched.
Which is why I only did a very basic first pass.

No worries, I'll extract them out and then squash the commits.

@dcousens
Copy link
Contributor Author

@weilu I squashed all the previous commits down and removed all changes to wallet.js.
I can squash it all to one commit if necessary, but the current breakdown seems more natural and easier to review.

I've also added in some changes I made to transaction.js which involved using Array.prototype.forEach rather than classic counter foreach loops.

I haven't noticed a clear pattern on coding style in regards to using forEach/map/filter versus more verbose alternatives; so I was curious as to your thoughts on that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a bug? (That is, type not being used)

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible. I have yet to test the multi-sig related methods. @dcousens are you keen on walking through the related methods and add necessary test cases?

@dcousens
Copy link
Contributor Author

Rebased on 9358a40 (current HEAD).

weilu added a commit that referenced this pull request Mar 24, 2014
Basic pass through JSHint
@weilu weilu merged commit c42e0fb into bitcoinjs:master Mar 24, 2014
@weilu
Copy link
Contributor

weilu commented Mar 24, 2014

Thanks @dcousens!

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