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
chore: standardize npm script names #19
Conversation
package.json
Outdated
@@ -17,13 +17,13 @@ | |||
], | |||
"scripts": { | |||
"lint": "eslint .", | |||
"test": "mocha ./tests/lib/**/*.js", | |||
"new-rule-format": "node ./bin/eslint-transforms.js -t ./lib/new-rule-format/new-rule-format.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ideas on what this script is doing? Per the new standard, with the exception of npm life cycle scripts all scripts should begin with build:
, release:
, lint:
, start:
, or test:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can prefix it with build:
, it seems to be updating rule from the old format to new format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a helper to run the transform on a file locally but isn't actually used as part of the build process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it doesn't seem to fit any of the build:
, release:
, lint:
, start:
, or test:
categories. Should we keep it as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just remove it.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @mdjermanovic to verify before merging.
Updated. |
Prerequisites checklist
What is the purpose of this pull request?
Refers eslint/eslint#14827
What changes did you make? (Give an overview)
This updates the names of the scripts in package.json to be consistent with the new standard.
Is there anything you'd like reviewers to focus on?
We may need to update the
release
related scripts on Jenkins before merging this PR.