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.

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

Merged
merged 11 commits into from Dec 14, 2022

Conversation

pmcelhaney
Copy link
Contributor

@pmcelhaney pmcelhaney commented Sep 15, 2022

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.)

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon chore This change is not user-facing labels Sep 15, 2022
@netlify
Copy link

netlify bot commented Sep 15, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 9ee75d7
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6398f4bac65f380009d5a8f7
😎 Deploy Preview https://deploy-preview-16315--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

package.json Outdated
Comment on lines 20 to 21
"build:release:publish": "node Makefile.js publishRelease",
"build:release:rc": "node Makefile.js generatePrerelease -- rc",
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'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.

Suggested change
"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",

Copy link
Sponsor Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.

automation > bikeshedding

Copy link
Contributor

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
Comment on lines 24 to 27
"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",
Copy link
Contributor Author

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.

@pmcelhaney
Copy link
Contributor Author

If this looks good, I'll do the other repos and submit all of the PRs at once.

package.json Outdated
Comment on lines 24 to 27
"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",
Copy link
Contributor Author

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.

Comment on lines -23 to -27
"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",
Copy link
Member

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.

Copy link
Contributor Author

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.

  1. Add the new names (without removing the old names).
  2. Update Jenkins to use the new names.
  3. A second commit that removes the old names.

Copy link
Member

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.

Copy link
Member

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.

@mdjermanovic
Copy link
Member

Our test script includes:

eslint/Makefile.js

Lines 631 to 638 in 8cc0bbe

target.test = function() {
target.lint();
target.checkRuleFiles();
target.mocha();
target.karma();
target.fuzz({ amount: 150, fuzzBrokenAutofixes: false });
target.checkLicenses();
};

Per the new standard, it shouldn't include linting, and checkRuleFiles and checkLicenses don't qualify as "execute code"?

Test

Scripts that execute code in order to ensure the actual behavior matches expected behavior MUST have names that begin with test.

If a package contains any test:* scripts, there SHOULD be a script named test and it MUST run of all of the tests that would have been run if each test:* script was called individually.

A test script SHOULD NOT include linting.

A test script SHOULD report test coverage when possible.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 15, 2022

Regarding the test script, I still think it's good that it runs all the checks, including linting, that need to pass before accepting changes or publishing a new release. It's convenient for contributors and easier for us to maintain (our release process runs npm test).

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 16, 2022
@pmcelhaney
Copy link
Contributor Author

pmcelhaney commented Sep 17, 2022

@mdjermanovic

Per the new standard, it shouldn't include linting

The spec says "SHOULD NOT" rather than "MUST NOT" so I think it's okay. :)

  1. SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that
    there may exist valid reasons in particular circumstances when the
    particular behavior is acceptable or even useful, but the full
    implications should be understood and the case carefully weighed
    before implementing any behavior described with this label.

https://www.ietf.org/rfc/rfc2119.txt

@mdjermanovic
Copy link
Member

Per the new standard, it shouldn't include linting

The spec says "SHOULD NOT" rather than "MUST NOT" so I think it's okay. :)

  1. SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that
    there may exist valid reasons in particular circumstances when the
    particular behavior is acceptable or even useful, but the full
    implications should be understood and the case carefully weighed
    before implementing any behavior described with this label.

https://www.ietf.org/rfc/rfc2119.txt

I think checkRuleFiles and checkLicenses can qualify as exceptions because they are specific to this project. But, linting is common to all our projects, and I'm not sure if there's a specific reason to include linting in the test script in the eslint/eslint project but not in other projects. And if we include linting in test scripts in all other projects, then it seems better to update the standard.

@nzakas
Copy link
Member

nzakas commented Sep 30, 2022

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.

@nzakas nzakas added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Nov 2, 2022
@nzakas
Copy link
Member

nzakas commented Nov 2, 2022

TSC Summary: This PR reformats our npm scripts to follow the standard we have already checked in.

TSC Questions:

  1. Do we want to rename the release-related scripts? We would just have to update the Jenkins job before doing the next release.
  2. Do we want to include linting in npm test?

@mdjermanovic
Copy link
Member

In the last TSC meeting, we agreed that:

  1. Release-related scripts should be renamed to follow the new standard, and we'll update Jenkins jobs accordingly. Details will be discussed in this PR.
  2. npm test will not include linting.

@mdjermanovic mdjermanovic removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Nov 9, 2022
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pmcelhaney and others added 2 commits November 11, 2022 18:57
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@pmcelhaney pmcelhaney force-pushed the 14827-standardize-npm-scripts branch 2 times, most recently from 8815a72 to 4a87cc1 Compare November 28, 2022 20:57
Copy link
Contributor Author

@pmcelhaney pmcelhaney left a 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.

Comment on lines +15 to 23
```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 )+
```
Copy link
Contributor Author

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.

Comment on lines 109 to 114
.language-abnf .token {
display: inline;
padding: 0;
margin: 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@mdjermanovic mdjermanovic left a 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:

target.lint();

@nzakas
Copy link
Member

nzakas commented Dec 8, 2022

@pmcelhaney are you still working on this?

@pmcelhaney pmcelhaney requested a review from a team as a code owner December 8, 2022 19:51
@@ -632,7 +632,6 @@ target.karma = () => {
};

target.test = function() {
target.lint();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate linting from testing.

@pmcelhaney
Copy link
Contributor Author

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.

Copy link
Member

@mdjermanovic mdjermanovic left a 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.

Copy link
Member

@nzakas nzakas left a 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?

@mdjermanovic
Copy link
Member

I'll update the Jenkins script right after merging this.

@mdjermanovic mdjermanovic merged commit ba74253 into eslint:main Dec 14, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 24, 2022
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 ([#&#8203;16637](eslint/eslint#16637)) (Daniel Bartholomae)
-   [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#&#8203;16654](eslint/eslint#16654)) (Sébastien Règne)

#### Bug Fixes

-   [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#&#8203;16579](eslint/eslint#16579)) (Nicholas C. Zakas)
-   [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#&#8203;16611](eslint/eslint#16611)) (Milos Djermanovic)

#### Documentation

-   [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#&#8203;16663](eslint/eslint#16663)) (Nicholas C. Zakas)
-   [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#&#8203;16563](eslint/eslint#16563)) (Ben Perlmutter)
-   [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#&#8203;16606](eslint/eslint#16606)) (Sam Chen)
-   [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#&#8203;16631](eslint/eslint#16631)) (Percy Ma)
-   [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#&#8203;16625](eslint/eslint#16625)) (Milos Djermanovic)
-   [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#&#8203;16628](eslint/eslint#16628)) (Karl Horky)
-   [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#&#8203;16591](eslint/eslint#16591)) (Tanuj Kanti)
-   [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#&#8203;16566](eslint/eslint#16566)) (Ben Perlmutter)
-   [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#&#8203;16607](eslint/eslint#16607)) (Pavel)
-   [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#&#8203;16603](eslint/eslint#16603)) (Sam Chen)

#### Chores

-   [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.0 ([#&#8203;16675](eslint/eslint#16675)) (Milos Djermanovic)
-   [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#&#8203;14827](eslint/eslint#14827) ([#&#8203;16315](eslint/eslint#16315)) (Patrick McElhaney)
-   [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#&#8203;16664](eslint/eslint#16664)) (Milos Djermanovic)
-   [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#&#8203;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>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 13, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants