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.

Sign up for GitHub

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

Jump to bottom

Replace script type strings with constants #679

Merged
merged 1 commit into from
Nov 4, 2016
Merged

Replace script type strings with constants #679

merged 1 commit into from
Nov 4, 2016

Conversation

afk11
Copy link
Contributor

@afk11 afk11 commented Oct 27, 2016

I find it's good practice to avoid repetitively using a string values throughout code.

  • It generally makes it easier for consumers of the library to check the correct value should they ever change
  • IDE's will complain if you've typed the wrong string

Also added an npm script to fix the code style: npm run standard-fixer

@jprichardson
Copy link
Member

ACK

dcousens
dcousens reviewed Oct 28, 2016
View reviewed changes
package.json
@@ -40,6 +40,7 @@
"coverage": "nyc --check-coverage --branches 90 --functions 90 mocha",
"integration": "mocha test/integration/",
"standard": "standard",
"standard-fixer": "standard --fix",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is unnecessary IMHO... it should rarely be needed...

dcousens
dcousens reviewed Oct 28, 2016
View reviewed changes
src/script.js
var OP_INT_BASE = OPS.OP_RESERVED // OP_1 - 1
var scriptTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a constant too? (caps)

@dcousens
Copy link
Contributor

dcousens commented Oct 28, 2016 •
edited
Loading

concept ACK, a few nits tho

@dcousens dcousens closed this Oct 28, 2016
@dcousens dcousens reopened this Oct 28, 2016
@dcousens dcousens modified the milestones: 3.1.0, 3.0.0 Oct 28, 2016
@dcousens
Copy link
Contributor

dcousens commented Oct 28, 2016 •
edited
Loading

@afk11 could you do your changes from branches within this repository?
I could resolve some of my naming nits pretty quickly and just get your final ACK rather than put the burden on you. 👍

@dcousens
Copy link
Contributor

dcousens commented Nov 2, 2016 •
edited
Loading

Needs rebase (though I'd wait on #682)

@dcousens dcousens changed the base branch from master to scriptconst November 4, 2016 00:01
@dcousens dcousens merged commit ab18b15 into bitcoinjs:scriptconst Nov 4, 2016
@dcousens
Copy link
Contributor

dcousens commented Nov 4, 2016

Merged into non-master branch so I can rebase it

@dcousens dcousens mentioned this pull request Nov 4, 2016
Merged
@Ashtonulliac Ashtonulliac mentioned this pull request Aug 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcousens dcousens dcousens left review comments

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@afk11 @jprichardson @dcousens