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

Additional option for skipping auto fixable rules #10957

Closed
okonet opened this issue Oct 11, 2018 · 5 comments
Closed

Additional option for skipping auto fixable rules #10957

okonet opened this issue Oct 11, 2018 · 5 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@okonet
Copy link

okonet commented Oct 11, 2018

The version of ESLint you are using.

v5.6.0

The problem you want to solve.

When ESLint is used with tools like lint-staged on pre-commit, it makes sense to not lint against auto-fixable rules during development process. This especially concerned about IDE integrations since such rules are generating a fair amount of noise in the IDEs. I honestly don't see any point of showing them if these errors are going to be fixed during pre-commit hook.

At the same, I'd like these rules to be enabled during the normal run, for example, on CI.

Your take on the correct solution to problem.

I think the simplest solution would be to add an additional flag (--skip-fixable-rules) to the CLI that would exclude autofixable rules automatically.

I could implement an alternative CLI runner myself using the API but there is at least one bug that prevents me from doing so.

Additionally, since the config has to be resolved per file, this means I would end up with something very similar to the current CLI runner.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 11, 2018
@kaicataldo kaicataldo added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 15, 2018
@kaicataldo
Copy link
Member

Thanks for the proposal. I have a few concerns:

  1. Since ESLint's autofixer avoids fixing when it might change the execution behavior, rules that can autofix are not guaranteed to fix every problem. By skipping these rules, there will be times when CI is the first time some errors that must be fixed by a human will be displayed. This might work for your workflow, but I know I'd personally rather fix things locally before pushing them and having to wait for CI to catch things I need to fix.
  2. Given that we have .eslintrc.js files, this can already be accomplished by using environment variables and a dynamically constructed configuration.

@okonet
Copy link
Author

okonet commented Oct 15, 2018

This might work for your workflow

That's why I propose a flag, not the default behavior

Given that we have .eslintrc.js files, this can already be accomplished by using environment variables and a dynamically constructed configuration.

I'd be interested in a solution that allow me doing this. AFAIK this is not possible at the moment.

@kaicataldo
Copy link
Member

kaicataldo commented Oct 28, 2018

That's why I propose a flag, not the default behavior

I would still be hesitant to introduce a new feature that feels broken on release. I think where I'm having trouble with the current proposal is in the following assumption:

I honestly don't see any point of showing them if these errors are going to be fixed during pre-commit hook.

ESLint doesn't guarantee all instances of errors will be fixed in a rule that has an autofixer. By adding a feature flag like this we're agreeing to supporting and maintaining this new feature, and I think most people would see it and assume that all instances of errors in rules with autofixers are fixable, which isn't the case.

I'd be interested in a solution that allow me doing this. AFAIK this is not possible at the moment.

I believe this would be feasible using ESLint's Node.js API to check if rules are fixable or not and filtering those that are fixable out of the config's list.

@nzakas @not-an-aardvark @platinumazure Do you mind giving this a look, since this is tangentially related to the proposal and discussions we've had for this proposal?

Let's see what the rest of the team has to say.

@nzakas
Copy link
Member

nzakas commented Oct 29, 2018

While I do agree that we need more control over when rules are and are not fixed (as in #10855), I'm not sure this proposal makes sense for most users.

As @kaicataldo points out, there is no guarantee that a fixable rule will apply any fixes to the output at all, so there is no clear distinction between a rule that will and will not apply fixes during precommit.

I also agree that the best way to trigger different behavior in different environments is to use a .eslintrc.js file with an environment variable that determines which rules to run. That was my first inclination when I read this proposal.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Dec 11, 2018
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 10, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

3 participants