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

Scripts: lint-styles --fix tries to fix all files when passing the list of files #30466

Open
scruffian opened this issue Apr 2, 2021 · 4 comments
Labels
Needs Dev Ready for, and needs developer efforts [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended

Comments

@scruffian
Copy link
Contributor

I'm trying to use wp-scripts to lint the Sass and CSS files in the themes repo. It works well to catch errors in staged files (using lint-staged) but when I pass the --fix argument to the command it tries to fix all the files in the repo, not just those that are staged. You can see the PR here: Automattic/themes#3611

cc @gziolo since you worked on #15890

@scruffian scruffian changed the title lint-styles --fix tries to fix all files lint-styles --fix tries to fix all files when called using lint-staged Apr 2, 2021
@gziolo
Copy link
Member

gziolo commented Apr 3, 2021

I don't immediately see examples of passing multiple files to stylelint in their docs: https://stylelint.io/user-guide/usage/cli.

@ntwb, do you know why the following in packages.json doesn't work as expected:

	"lint-staged": {
 		"*.{css,scss}": [
 			"wp-scripts lint-style --fix"
 		]
 	},

I can reproduce it also in Gutenberg.

@gziolo gziolo added [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended labels Apr 3, 2021
@gziolo gziolo changed the title lint-styles --fix tries to fix all files when called using lint-staged Sxripts: lint-styles --fix tries to fix all files when passing the list of files Apr 3, 2021
@aristath aristath changed the title Sxripts: lint-styles --fix tries to fix all files when passing the list of files Scripts: lint-styles --fix tries to fix all files when passing the list of files Apr 5, 2021
@gziolo
Copy link
Member

gziolo commented Apr 6, 2021

It looks like the implementation of minimist isn't ideal in:

const getFileArgsFromCLI = () => minimist( getArgsFromCLI() )._;

Screen Shot 2021-04-06 at 09 43 25

So the issue is that it assumes that --fix is a modifier that can be followed up with a value so it completely ignores the first path passed. The quick way to make it work could be the following:

wp-scripts lint-style --fix=

In general, if there is more than one file passed by lint-staged it should work without any changes for all passed files but the first one ...

@gziolo gziolo added the Needs Dev Ready for, and needs developer efforts label Apr 6, 2021
@gziolo gziolo added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Feb 11, 2022
@t-hamano
Copy link
Contributor

@gziolo

I added the option ({boolean: true}) and it appears to have parsed correctly.
Is this approach appropriate? 🤔

const minimist = require("minimist")

// Not correctly parsed...
console.log( minimist(['--fix', 'aaa.scss', 'bbb.scss', 'ccc.scss'])._ );
// Correctly parsed (temporary Fix equal sign)
console.log( minimist(['--fix=', 'aaa.scss', 'bbb.scss', 'ccc.scss'])._ );
// Correctly parsed (use boolean option)
console.log( minimist(['--fix', 'aaa.scss', 'bbb.scss', 'ccc.scss'], {boolean: true})._ );

minimist

@gziolo
Copy link
Member

gziolo commented Jul 20, 2022

@t-hamano, that looks promising. Let's try that approach 👍🏻

Good news is that native experimental args parsing came to Node 18.3:

https://nodejs.org/api/util.html#utilparseargsconfig

They develop it also as a package available on npm: https://github.com/pkgjs/parseargs.

@gziolo gziolo removed the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Dev Ready for, and needs developer efforts [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants