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

should throw an error when disabling a non-existent rule #9505

Closed
aladdin-add opened this issue Oct 23, 2017 · 33 comments
Closed

should throw an error when disabling a non-existent rule #9505

aladdin-add opened this issue Oct 23, 2017 · 33 comments

Comments

@aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Oct 23, 2017

Tell us about your environment

  • **ESLint Version:**master
  • Node Version:
  • npm Version:

What parser (default, Babel-ESLint, etc.) are you using?
default

What did you do? Please include the actual source code causing the issue.

/* eslint foo: 0 */

// edit: or in a config file
{
  "rules": { "foo": 0}
}

What did you expect to happen?
throw an error, since there is no rule named foo.
What actually happened? Please include the actual, raw output from ESLint.
no error thrown.

@aladdin-add aladdin-add changed the title should throw an error when a rule config is not valid. should throw an error when a rule config in comment is not valid. Oct 23, 2017
@aladdin-add aladdin-add changed the title should throw an error when a rule config in comment is not valid. should throw an error when config a non-existent rule Oct 23, 2017
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Oct 23, 2017

I can't reproduce this in the demo:

Loading

@aladdin-add
Copy link
Member Author

@aladdin-add aladdin-add commented Oct 24, 2017

interesting! seems only happen when disabling a non-existent rule.(same issue in config file)

image

Loading

@aladdin-add aladdin-add changed the title should throw an error when config a non-existent rule should throw an error when disabling a non-existent rule Oct 24, 2017
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Oct 24, 2017

Does this also happen in a config file?

{
  "rules": {
    "foo": 0
  }
}

Loading

@aladdin-add
Copy link
Member Author

@aladdin-add aladdin-add commented Oct 24, 2017

yes, I guess it might be unexpected.

Loading

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Oct 24, 2017

This might be caused by #7690. I agree that reporting an error nonexistent rules might be a good idea.

Also related: #9373

Loading

@aladdin-add
Copy link
Member Author

@aladdin-add aladdin-add commented Apr 1, 2018

seems we are missing this issue, hopefully it can be fixed in eslint v5, since it would be a breaking change.

friendly ping @not-an-aardvark

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 24, 2018

This seems like a nasty bug that we should fix with the next major release.

Loading

@nzakas nzakas added this to the v6.0.0 milestone Oct 24, 2018
@not-an-aardvark not-an-aardvark added this to Accepted, ready to implement in v6.0.0 Oct 24, 2018
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Oct 24, 2018

As a minor note for whoever implements this: I think the invalid comment should be reported as a linting problem, not by throwing an error. In general we've been trying to preserve the invariant that issues with the source text shouldn't cause any errors to be thrown, since this prevents integrations from crashing if the integrations are supposed to handle arbitrary text from the user. (For example, see #9819 and eslint/website#413.)

As another minor note, we've generally been using Projects rather than Milestones to track changes for major releases -- for example, I just added this issue to the v6.0.0 project.

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 26, 2018

Loading

@sstern6
Copy link
Contributor

@sstern6 sstern6 commented Nov 12, 2018

Would like to take this on, though I have a few questions.

But before i get into it, @aladdin-add have you started working on it?

Loading

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 13, 2018

@sstern6 Though you can feel free to make a PR, we're most likely quite a ways out from our next major release. Since this is a breaking change, we wouldn't be able to merge it until we start working on that release in earnest. My recommendation would be to hold off for now until that process starts.

Loading

@sstern6
Copy link
Contributor

@sstern6 sstern6 commented Nov 13, 2018

thanks @kaicataldo Ill hold off. Plenty of other stuff I can get done.

Loading

@sstern6
Copy link
Contributor

@sstern6 sstern6 commented Dec 4, 2018

@kaicataldo because were on 5.9.0, should we start considering how to handle this for the major release?

Loading

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented May 9, 2019

@ljharb Would a warning for non-existing rule be an issue for you? As in, if you disabled a rule in AirBnB that was added in 6.1 and somebody runs your config in 6.0, it would display a warning saying that the rule doesn't exist.

Loading

@ljharb
Copy link
Contributor

@ljharb ljharb commented May 9, 2019

As long as there was a way to silence the warning (separate from --quiet, but I’d expect quiet to silence them too), that seems tolerable - but what would be better is a way to do that directly in the config, like “i know this might not be a rule but hush, i know what I’m doing”.

Loading

@btmills
Copy link
Member

@btmills btmills commented May 9, 2019

We discussed this in today's TSC meeting, and the conclusion was that we should be safe throwing an error if someone disables a non-existent rule in a configuration comment in 6.0.0. We'll leave configuration files (and plugins) alone for this version and see if there's a solution that works for end users and plugin authors to be defined later.

Loading

@ljharb
Copy link
Contributor

@ljharb ljharb commented May 9, 2019

That sounds fine for my use cases, thanks for considering them!

Loading

@aladdin-add
Copy link
Member Author

@aladdin-add aladdin-add commented May 14, 2019

working on this.

Loading

aladdin-add referenced this issue in aladdin-add/eslint May 19, 2019
aladdin-add referenced this issue in aladdin-add/eslint May 19, 2019
@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 May 20, 2019
aladdin-add referenced this issue in aladdin-add/eslint May 22, 2019
@mysticatea mysticatea moved this from Implemented, pending review to Accepted, ready to implement in v6.0.0 May 24, 2019
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 24, 2019
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 24, 2019
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 24, 2019
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 24, 2019
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 24, 2019
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases
@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 May 24, 2019
v6.0.0 automation moved this from Implemented, pending review to Done May 25, 2019
ilyavolodin added a commit that referenced this issue May 25, 2019
* Breaking: stricter rule config validating (fixes #9505)

to fix issue #9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases

* Update linter.js

* Update linter.js
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v6.0.0
  
Done
10 participants