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

fix!: Behavior of CLI when no arguments are passed #17644

Merged
merged 13 commits into from
Dec 21, 2023
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Oct 12, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[x] 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
[ ] Other, please explain:

What changes did you make? (Give an overview)

When running the CLI without file arguments it silently fails right now. This PR makes the following changes:

  • When running the CLI in eslintrc mode, failing to use file arguments throws an error.
  • When running the CLI in flat config mode, failing to use file arguments means that it runs on the current working directory.

Fixes #14308

Is there anything you'd like reviewers to focus on?

To make this work, I changed how lintFiles() works in both ESLint and FlatESLint. In FlatESLint, I decided that an empty string and an empty array are the same and should be treated as .. I'm not sure if that's the right decision, so I'd like to hear some opinions on that.

@nzakas nzakas requested a review from a team as a code owner October 12, 2023 16:22
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Oct 12, 2023
@netlify
Copy link

netlify bot commented Oct 12, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit df221bb
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/658367ecab1c410007859b33

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Oct 13, 2023
@mdjermanovic
Copy link
Member

I decided that an empty string and an empty array are the same and should be treated as .. I'm not sure if that's the right decision, so I'd like to hear some opinions on that.

Makes sense to me.

lib/eslint/flat-eslint.js Outdated Show resolved Hide resolved
lib/eslint/eslint-helpers.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented Oct 13, 2023

Fixed up the edge cases. I also made it so FlatESLint#lintFiles() doesn't require an argument, and if it's missing, then we use ["."]. This is to have no argument behave the same as "" and [], which makes logical sense to me.

@fasttime
Copy link
Member

Let me point out a use case that I think will be negatively impacted by this change. Sometimes people run ESLint only on a subset of files that is calculated dynamically and that could be possibly empty. For example, for performance reasons, some users will want to lint only changed files in a git repo, so they could be running a command like this (reference):

eslint --fix $(git diff --name-only HEAD | xargs)

After this PR, if a user runs the above command while there are no uncommitted changes, the whole current directory will be linted (or they will get an error if eslintrc is used).

@nzakas
Copy link
Member Author

nzakas commented Oct 13, 2023

@fasttime for clarification, you're referring to the flat config behavior, which defaults to . and not to the eslintrc behavior, which throws an error.

I assume that ESLint isn't the only tool people are using in this way. There are a lot of tools that default to . when no file arguments are present. How do other tools handle this use case?

@fasttime
Copy link
Member

I assume that ESLint isn't the only tool people are using in this way. There are a lot of tools that default to . when no file arguments are present. How do other tools handle this use case?

I'd guess that people are using ESLint like this simply because they were able do it so far. npx prettier prints usage info and exits with an error code. The same happens when I run javac (the Java compiler binary) without arguments. On the other hand, ls without arguments just lists the contents of the current directory, so probably there is no standard way to handle this edge case.

@nzakas
Copy link
Member Author

nzakas commented Oct 16, 2023

I think jest works in such a way that you can either pass no arguments or specific tests? Maybe there's some way to set that up for a precommit hook that we can learn from? (I'm not familiar with Jest, so looking for some insights.)

@fasttime
Copy link
Member

In fact running jest without arguments prints an error message and exits with an error code:

PS D:\foo> npx jest                  
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In D:\foo
  8 files checked.
  testMatch: **/__tests__/**/*.[jt]s?(x), **/?(*.)+(spec|test).[tj]s?(x) - 0 matches
  testPathIgnorePatterns: \\node_modules\\ - 8 matches
  testRegex:  - 0 matches
Pattern:  - 0 matches

But this behavior can be changed with a flag (--passwithnotests):

PS D:\foo> npx jest --passWithNoTests
No tests found, exiting with code 0

@fasttime
Copy link
Member

Maybe there's some way to set that up for a precommit hook that we can learn from?

It seems that lint-staged does not run tasks for glob patterns that don't match any staged file, so at least it shouldn't be a concern for this change.

@nzakas
Copy link
Member Author

nzakas commented Oct 18, 2023

Jest is failing without tests because it didn't find any in the directories that it searches by default. It seems like --passwithnotests might be the best thing to copy.

@nzakas
Copy link
Member Author

nzakas commented Oct 18, 2023

TSC Summary: This PR changes the default behavior of the ESLint CLI when no file arguments are passed in. In eslintrc mode, the CLI now reports an error; in flat config mode, the CLI now assumes . as the file argument. This may cause a problem for people trying to use the CLI as part of a precommit check. Before this change, the CLI would exit with a zero exit code when no file arguments were present, after this change, it will either error or lint every file, neither of which seems desirable.

TSC Questions:

  1. This now looks like a breaking change, should we add it to v9? (This was originally intended to be completed with the flat config work, so it should have been included?)
  2. Do we want to provide a way to mimic the current behavior? Jest has a --passwithnotests CLI flag that we could mimic for this purpose.

@nzakas nzakas added tsc agenda This issue will be discussed by ESLint's TSC at the next meeting breaking This change is backwards-incompatible labels Oct 18, 2023
@nzakas nzakas marked this pull request as draft October 19, 2023 20:15
@sam3k sam3k removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Oct 20, 2023
@github-actions
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 30, 2023
@fasttime fasttime removed the Stale label Oct 31, 2023
@nzakas nzakas self-assigned this Oct 31, 2023
@nzakas
Copy link
Member Author

nzakas commented Nov 30, 2023

@sam3k please be sure to leave comments as to the resolution from the TSC meeting along with removing the "tsc agenda" label.

@sam3k
Copy link
Contributor

sam3k commented Nov 30, 2023

@sam3k please be sure to leave comments as to the resolution from the TSC meeting along with removing the "tsc agenda" label.

Sounds good

@nzakas
Copy link
Member Author

nzakas commented Dec 6, 2023

@mdjermanovic this is ready for review

@mdjermanovic mdjermanovic changed the title fix: Behavior of CLI when no arguments are passed fix!: Behavior of CLI when no arguments are passed Dec 8, 2023
lib/eslint/eslint.js Outdated Show resolved Hide resolved
lib/options.js Outdated Show resolved Hide resolved
docs/src/use/command-line-interface.md Outdated Show resolved Hide resolved
tests/lib/eslint/eslint.js Show resolved Hide resolved
tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
tests/lib/cli.js Outdated Show resolved Hide resolved
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 19, 2024
@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 19, 2024
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 breaking This change is backwards-incompatible bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Omiting any file/dir positional parameter to CLI does not give an error
4 participants