Tools: Standardize script naming in main package.json #42368
Conversation
t-hamano
changed the title
|
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
t-hamano
changed the title
t-hamano
requested review from
gziolo,
ntwb,
nerrad,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Awesome work so far @t-hamano. I like how you used external tools to drive the new naming schema and to introduce best practices. |
docs/contributors/code/react-native/getting-started-react-native.md
Outdated
Show resolved
Hide resolved
t-hamano
changed the title
|
That was a quick round of fixes 👏🏻 I'm happy with the current state of the art. I would appreciate wider feedback from other people in the project. There is this functionality in Node.js that shows suggested script names when you type something that doesn’t exist. It should help with the transition:
|
|
Thanks for the thoughtful advice. Should we update these test commands or reinstate |
Yes, I was looking at that. Let's bring it back to remove friction and to keep the discussion focused on more important changes 😄 |
|
I love this improvement. Thank you, @t-hamano! I usually start typing P.S. I don't remember if it's a Node.js feature or my Screenshot
|
|
This needs some more testing; otherwise, it seems ready. We asked for feedback during the core editor chat last week, and it looks like nobody was opposed to the idea. |
|
This is great @t-hamano, thank you for working on it! Once concern I have is that this could break external code that relies on the old task names. I don't think it's a blocker, but I wonder if you have any thoughts about that. |
|
@adamziel However, I'm a little concerned about releasing to the plugin repo and publishing npm packages because we cannot test them. @gziolo |
|
The following command names were affected: I ran The tests, the linting, and building the packages are accounted for in the CI checks. I ran some commands including @t-hamano You may want to write a make core post to a) make others aware b) help them easily find the cause any problems that we may have missed here. Other than that, I think we're good. Great job! |
There was a problem hiding this comment.
I tested several scripts that have a new name, and everything works as expected.
I suggested two more changes to the script names.
You may want to write a make core post to a) make others aware b) help them easily find the cause any problems that we may have missed here. Other than that, I think we're good. Great job!
Excellent idea. Let's do that based on the description from the post, so all contributors are aware of the changes applied. I don't expect people to remember all those script names, but some of them might get memorized 😄
The best part is that those names align now with what we have in WordPress core:
I struggled personally with npm run test:php vs npm run test-php.
|
Thanks for the review and the advice!
I don't have Make WordPress Core posting permissions, should I request permissions on Slack for this? Also, I will first correct the items pointed out by @gziolo . |
Yes!
Post the link on slack and also feel free to ping me – I'm happy to help! |
It's also fine to draft the post using markdown and share it here. We can post on your behalf, so you don't have to go through the whole learning process for one-time publishing. If you start writing regularly, then you will definitely need access to the Make WordPress Core blog. When you merge this PR, make sure to let people know in the WordPress Slack in the |
|
✅ Corrected the points raised in your review. If it passes the test again, I will let you know via Slack after the merge. And the draft was also prepared with the following content: Standardize npm script naming in the Gutenberg projectThis is an announcement to all contributors to the Gutenberg project. The Gutenberg project has a number of To standardize scripts naming, scriptlint was used as the base rule. Mandatory-start, mandatory-dev and mandatory-test
Use namespaceUsing namespaced script names improves readability and makes the intent more obvious. Namespaces are used to group related scripts, and a colon is used to separate namespaces. Based on this rule, all scripts were given namespaces, with a few exceptions. Here are some examples:
Alphabetic orderSorting script names alphabetically makes it easier to find a particular script. The Unify parallel commandsThe way of parallelizing commands was a mixture of connecting them with & and npm-run-all package, but scripts were unified to use npm-run-all as possible. A list of new scripts can be found at this link. (...link to package.json scripts in GitHub) For more info see #19949 and #42368. Props @mkaz for suggestion, @gziolo, @adamziel, and @Mamaduka for reviewing. |
|
@t-hamano great job! I tool a liberty of editing your comment – you can see the revision history to see what I've changed. I do not understand these two bullet points, though:
It would be great to rewrite them somehow. What should have at least |
|
Thank you for your peer review!
I agree this may not be clear to those who see it for the first time. To clarify the changes, I have divided each into sections and explained them more concretely with examples. |
Fix: #19949
What?
This PR Standardizes script naming in main
package.jsonwith scriptlint as the base rule.I would like to get more eyes to find any problems because this PR is renaming scripts in a wide range of areas.
Why?
The main
package.jsonhas undergone a number of changes, which have resulted in a larger number of scripts.I think it is necessary to organize these scripts according to certain rules, both for those who see these scripts for the first time and to improve code quality.
How?
I have changed the script based on some rules in scriptlint.
At the same time, I also changed the related documentation and shell scripts to be consistent.
Minimum Rules Applied
mandatory-start, mandatory-dev and mandatory-test
As a minimum rule,
start,test, anddevscripts are required.I defined alias for
devas a script that corresponds tostart.Strict Rules Applied
uses-allowed-namespace
I have redefined namespace as follows, with the exception of Life Cycle Scripts:
buildcleandevdocsenvfixturesformatlintotherpublishstartstorybooktestwp-envHowever, strictly speaking, a script must be defined that serves as the root namespace, which I ignored.
(Example:
docs:blocksscript must have a parentdocsscript defined)no-unix-double-ampersand / no-unix-single-ampersand
I rewrote it using
npm-run-all, but some scripts were left ampersanded since this package only allows you to specify defined scriptsOptional rules
natural-order
A stricter rule is alphabetic-order, but scripts with a
pre-orpost-prefix are unnaturally lined up, so I applied this rule.Rules not adopted
correct-casing
This would require changing the script name to
camelCase, which seemed very unnatural, so I ignored it.other:check-local-changestoother:checkLocalChangestest:create-blocktotest:createBlockwp-envtowpEnvno-aliases
wp-envfalls into this category, but I left alias as backward compatible:Running Tests
After applying the above rules, you can check what warnings remain by using the following command:
See Result