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 per #14827 #16315
chore: standardize npm script names per #14827 #16315
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
package.json
Outdated
"build:release:publish": "node Makefile.js publishRelease", | ||
"build:release:rc": "node Makefile.js generatePrerelease -- rc", |
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.
It's a bit confusing that "publish" comes before "rc". The standard (which I wrote) says alphabetical order. May want to update the standard to make room for nuances like this.
"build:release:publish": "node Makefile.js publishRelease", | |
"build:release:rc": "node Makefile.js generatePrerelease -- rc", | |
"build:release:rc": "node Makefile.js generatePrerelease -- rc", | |
"build:release:publish": "node Makefile.js publishRelease", |
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 understand that there are one-off cases where the ordering can be unusual, but personally, I don't think we should have exceptions to alphabetical ordering. We should just enable prefer-alphabetical-scripts from npm-package-json-lint and call it a day so there's no need to debate the ordering.
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.
Makes sense.
automation > bikeshedding
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.
Running sort-package-json
's --check
flag in CI can also help here I think
package.json
Outdated
"lint": "node Makefile.js lint", | ||
"lint:docs:js": "node Makefile.js lintDocsJS", | ||
"lint:docs:js:fix": "node Makefile.js lintDocsJS -- fix", | ||
"lint:fix": "node Makefile.js lint -- fix", |
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.
VSCode's sorting algorithm wants to put the longer strings first.
"lint:docs:js:fix": "node Makefile.js lintDocsJS -- fix",
"lint:docs:js": "node Makefile.js lintDocsJS",
"lint:fix": "node Makefile.js lint -- fix",
"lint": "node Makefile.js lint",
Calling it out for the sake of curiosity. That's definitely wrong compared to what I was taught in grade school, but maybe VSCode has a good reason.
If this looks good, I'll do the other repos and submit all of the PRs at once. |
package.json
Outdated
"lint": "node Makefile.js lint", | ||
"lint:fix": "node Makefile.js lint -- fix", | ||
"lint:docs:js": "node Makefile.js lintDocsJS", | ||
"lint:docs:js:fix": "node Makefile.js lintDocsJS -- fix", |
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.
This is not alphabetical because "lint:f" should come after "lint:d". If we write a validator at some point we'll need to fuss over these details.
"generate-release": "node Makefile.js generateRelease", | ||
"generate-alpharelease": "node Makefile.js generatePrerelease -- alpha", | ||
"generate-betarelease": "node Makefile.js generatePrerelease -- beta", | ||
"generate-rcrelease": "node Makefile.js generatePrerelease -- rc", | ||
"publish-release": "node Makefile.js publishRelease", |
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.
These scripts are used on Jenkins, so we need to decide if we'll make a change there or keep these names here.
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 don't know how complex / risky it is to update Jenkins. If you decide to use the new names, might want to do it like this.
- Add the new names (without removing the old names).
- Update Jenkins to use the new names.
- A second commit that removes the old names.
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.
Let’s just leave these as-is right now. These aren’t just used within Jenkins, they are baked into eslint-release, which is used by all of our packages, making changing these script names more complex.
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.
Sorry, was too tired when I wrote this. The only place using these command is in Jenkins, and so long as we update Jenkins before merging this, I don’t see a problem.
Our Lines 631 to 638 in 8cc0bbe
Per the new standard, it shouldn't include linting, and
|
Regarding the |
The spec says "SHOULD NOT" rather than "MUST NOT" so I think it's okay. :)
|
I think |
I’d like to exclude linting from “test”. In my own workflow, I typically do linting after all tests pass. Plus, between our CI and precommit hooks, we aren’t in danger of missing it. |
TSC Summary: This PR reformats our npm scripts to follow the standard we have already checked in. TSC Questions:
|
In the last TSC meeting, we agreed that:
|
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
8815a72
to
4a87cc1
Compare
4a87cc1
to
bb930b4
Compare
d629dda
to
c7007c3
Compare
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.
Finally got around to making the change for "release:" scripts.
```abnf | ||
name = life-cycle / main target? option* ":watch"? | ||
life-cycle = "prepare" / "preinstall" / "install" / "postinstall" / "prepublish" / "preprepare" / "prepare" / "postprepare" / "prepack" / "postpack" / "prepublishOnly" | ||
main = "build" / "lint" ":fix"? / "release" / "start" / "test" | ||
target = ":" word ("-" word)* / extension ("+" extension)* | ||
option = ":" word ("-" word)* | ||
word = ALPHA + | ||
extension = ( ALPHA / DIGIT )+ | ||
``` |
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.
This was a rough sketch working from memory. I looked up the grammar for ABNF and rewrote it correctly.
.language-abnf .token { | ||
display: inline; | ||
padding: 0; | ||
margin: 0; | ||
} | ||
|
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.
To fix formatting on the web site: https://eslint.org/docs/latest/developer-guide/package-json-conventions
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.
Changes in package.json
and the docs LGTM. I think the only thing left to do in this PR is to exclude linting from npm test
by removing this line:
Line 633 in 49a07c5
target.lint(); |
@pmcelhaney are you still working on this? |
@@ -632,7 +632,6 @@ target.karma = () => { | |||
}; | |||
|
|||
target.test = function() { | |||
target.lint(); |
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.
Separate linting from testing.
Sorry, I missed your comment, @mdjermanovic. Removed that line so lint is no longer included in test. @nzakas This is ready to go as far as I know. Once it's merged I'll start working on updating the other repos. |
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, thanks! Leaving open for others to review.
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. Don't want to merge until we have a plan for updating the Jenkins script.
@mdjermanovic do you want to do that as part of this week's release?
I'll update the Jenkins script right after merging this. |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.29.0` -> `8.30.0`](https://renovatebot.com/diffs/npm/eslint/8.29.0/8.30.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.30.0`](https://github.com/eslint/eslint/releases/tag/v8.30.0) [Compare Source](eslint/eslint@v8.29.0...v8.30.0) #### Features - [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#​16637](eslint/eslint#16637)) (Daniel Bartholomae) - [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#​16654](eslint/eslint#16654)) (Sébastien Règne) #### Bug Fixes - [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#​16579](eslint/eslint#16579)) (Nicholas C. Zakas) - [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#​16611](eslint/eslint#16611)) (Milos Djermanovic) #### Documentation - [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#​16663](eslint/eslint#16663)) (Nicholas C. Zakas) - [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#​16563](eslint/eslint#16563)) (Ben Perlmutter) - [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#​16606](eslint/eslint#16606)) (Sam Chen) - [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#​16631](eslint/eslint#16631)) (Percy Ma) - [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#​16625](eslint/eslint#16625)) (Milos Djermanovic) - [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#​16628](eslint/eslint#16628)) (Karl Horky) - [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#​16591](eslint/eslint#16591)) (Tanuj Kanti) - [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#​16566](eslint/eslint#16566)) (Ben Perlmutter) - [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#​16607](eslint/eslint#16607)) (Pavel) - [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#​16603](eslint/eslint#16603)) (Sam Chen) #### Chores - [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@​eslint/eslintrc](https://github.com/eslint/eslintrc)[@​1](https://github.com/1).4.0 ([#​16675](eslint/eslint#16675)) (Milos Djermanovic) - [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#​14827](eslint/eslint#14827) ([#​16315](eslint/eslint#16315)) (Patrick McElhaney) - [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#​16664](eslint/eslint#16664)) (Milos Djermanovic) - [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#​16618](eslint/eslint#16618)) (Bryan Mishkin) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC40IiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
See #14827. 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?
I don't know if there are any scripts that reference these script names. I couldn't find any within the repo itself. (Edit: see comments, some of them are called by Jenkins.)