Scripts: fix --fix doesn't work correctly
#42578
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
force-pushed
the
fix/scripts-lint-fix
branch
from
261b0f7 to
42ee3fb
Compare
t-hamano
changed the title
--fix doesn't work correctly
|
Size Change: 0 B Total Size: 1.26 MB ℹ️ View Unchanged
|
t-hamano
added
[Type] Bug
t-hamano
requested review from
gziolo,
ntwb,
nerrad,
ajitbohra and
ryanwelcher
as code owners
gziolo
reviewed
Jul 21, 2022
| @@ -21,7 +21,8 @@ const getArgFromCLI = ( arg ) => { | |||
|
|
|||
| const hasArgInCLI = ( arg ) => getArgFromCLI( arg ) !== undefined; | |||
|
|
|||
| const getFileArgsFromCLI = () => minimist( getArgsFromCLI() )._; | |||
| const getFileArgsFromCLI = () => | |||
There was a problem hiding this comment.
I'm a bit worried that it won't work for every tool. Maybe, we should make it an option you pass so we don't break anything accidentally?
Example:
Line 287 in 1a33c0d
Here it would detect the path to the config as a file passed to the CLI.
I'm afraid that every tool has something like that. I took a look at stylelint docs, and they also allow some liberty on how to define options that take values:
https://stylelint.io/user-guide/usage/cli
Example:
stylelint "foo/**/*.scss" --cache --cache-location "/Users/user/.stylelintcache/"
The trick they do is placing the list of files immediately after the name of the tool.
It's really difficult to tell if there is any good solution here. It looks like the boolean flag just before the list of files is the only issue.
There was a problem hiding this comment.
I'm a bit worried that it won't work for every tool. Maybe, we should make it an option you pass so we don't break anything accidentally?
Indeed it is.
How about providing a default option as follows?
const getFileArgsFromCLI = ( opts = {} ) => minimist( getArgsFromCLI(), opts )._;
or...
const getFileArgsFromCLI = ( useBoolOpt = false ) =>
minimist( getArgsFromCLI(), useBoolOpt ? { boolean: true } : {} )._;It's really difficult to tell if there is any good solution here. It looks like the boolean flag just before the list of files is the only issue.
Would it work to explicitly handle --fix parameters and target files and pass them to the command in the correct order? 🤔
For example, wp-scripts format seems to explicitly control parameters and order as follows:
gutenberg/packages/scripts/scripts/format.js
Lines 95 to 114 in 1a33c0d
There was a problem hiding this comment.
For example, wp-scripts format seems to explicitly control parameters and order as follows:
I don't think the order was explicitly picked because of the issue we discuss.
We try to parse the command to guess with minimist whether the list of files is provided. We use that so people don't have to provide the default search patterns that differ between tools. In general, it works great, but we have this edge case where minimist isn't smart enough to recognise whether the boolean param like --fix should be followed with the value or not. This is also why the hack I recommended with using --fix= resolves the issue.
There was a problem hiding this comment.
Thank you, it is a more complicated issue than I had thought.
Regarding the solution of this issue, would it be better to wait until the native parser of Node 18 become stable and usable, as you commented?
If so, I would like to close this PR once and for all 👍
There was a problem hiding this comment.
I guess we better leave it as is, so we don't introduce other bugs. We can keep the issue open until someone finds a magic solution. In the meantime, I better remove Good First Issue label as it obviously isn't a good fit anymore 😄
There was a problem hiding this comment.
Thank you so much for trying this approach. It looked so promising at first check!
|
I'm curious whether it wouldn't be safer to document how to ensure that listed files are always correctly recognised:
|
|
I believe the I think it would be good to treat |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #30466
What?
This PR corrects the
--fixoption of wp-scripts so that the specified file is correctly reflected.Why?
If we don't specify the target file, all files are covered and itworks as expected:
However, as mentioned in comment, if some files are specified, it should be interpreted as follows by the external library minimist, and all files should be covered.
How?
I found an option in minimist to properly handle double hyphenated arguments, so I applied it.
Testing Instructions
If you have the auto-formatting feature enabled in your code editor, turn it off once.
lint-js
test1.js,test2.js, andtest3.js, and write the following code:run
node ./packages/scripts/scripts/lint-js.js --fix test1.js test2.jsOnly the two files you specify should be formatted as follows:
lint-style
test1.css,test2.css, andtest3.css, and write the following code:run
node ./packages/scripts/scripts/lint-style.js --fix test1.css test2.cssOnly the two files you specify should be formatted as follows: