-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Fix default use of lint-js file patterns #16079
Conversation
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.
@aduth thanks for catching this issue quite quickly. This was the worst rebase ever 🙈 I added a commit which fixes new ESLint issues introduced since the original change landed.
I retested all commands with some debugging to ensure that CLI params are handled properly again.
@@ -18,7 +18,7 @@ const { | |||
|
|||
const args = getCliArgs(); | |||
|
|||
const defaultFilesArgs = ! hasFileInCliArgs ? [ '.' ] : []; |
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.
Couple asides:
- I changed the logic of this expression since if I personally find that "if not x, else" is generally more confusing to follow in than "if x, else". Seems like a good general rule of thumb, but open to counter-points, since maybe it could be argued that as assigning a default value, the most relevant logic path is the absence of the CLI argument. Admittedly I probably shouldn't have made the refactor here vs. getting the fix in place.
- This function is technically not named correctly in accordance with naming guidelines. It should be
hasFileInCLIArgs
("CLI" is an acronym).
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 function is technically not named correctly in accordance with naming guidelines. It should be
hasFileInCLIArgs
("CLI" is an acronym).
I will fix it in a follow-up PR, I bet there are more functions to update 😅
I'm fine with "if x, else" it reads better in general. PR was small enough to include it and The Boy Scout Rule always applies 👍
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 will fix it in a follow-up PR, I bet there are more functions to update 😅
Ready: #16091.
Issue introduced in: #15890
This pull request seeks to resolve an issue where the
lint-js
script does not work in default usage. Without these changes, runningnpm run lint-js
will output ESLint's default usage instructions.As mentioned by @gziolo in Slack, the
hasFileInCliArgs
utility should be called as a function. These changes are included here.Further, as mentioned at , there appears to be an issue with the implementation of
lint-pkg-json.js
which would have prevented the default file arguments from being included correctly in the spread array of arguments. This has been resolved in 92ed212.Testing Instructions:
Verify
npm run lint-js
works as expected.WARNING: In my testing, I am encountering issues with memory exhaustion which I can't yet explain. Based on my understanding of these errors, it may be scanning recursively or folders which it is not expecting to. I am unable yet to understand why this would be happening now as a result of #15890 (or even also #15977).