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 · Fixed by #11742 · May be fixed by Omrisnyk/npm-lockfiles#130
Closed

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

aladdin-add opened this issue Oct 23, 2017 · 33 comments · Fixed by #11742 · May be fixed by Omrisnyk/npm-lockfiles#130
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 core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Milestone

Comments

@aladdin-add
Copy link
Member

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.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Oct 23, 2017
@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 added bug ESLint is working incorrectly 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 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

I can't reproduce this in the demo:

@aladdin-add
Copy link
Member Author

aladdin-add commented Oct 24, 2017

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

image

@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

Does this also happen in a config file?

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

@aladdin-add
Copy link
Member Author

yes, I guess it might be unexpected.

@not-an-aardvark
Copy link
Member

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

Also related: #9373

@aladdin-add
Copy link
Member Author

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

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 24, 2018
@nzakas
Copy link
Member

nzakas commented Oct 24, 2018

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

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

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/archive-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.

@nzakas
Copy link
Member

nzakas commented Oct 26, 2018 via email

@sstern6
Copy link
Contributor

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?

@kaicataldo
Copy link
Member

@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.

@sstern6
Copy link
Contributor

sstern6 commented Nov 13, 2018

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

@sstern6
Copy link
Contributor

sstern6 commented Dec 4, 2018

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

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting and removed bug ESLint is working incorrectly labels Apr 27, 2019
@ilyavolodin
Copy link
Member

@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.

@ljharb
Copy link
Sponsor Contributor

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”.

@btmills
Copy link
Member

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.

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels May 9, 2019
@ljharb
Copy link
Sponsor Contributor

ljharb commented May 9, 2019

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

@aladdin-add
Copy link
Member Author

working on this.

aladdin-add referenced this issue in aladdin-add/eslint May 19, 2019
aladdin-add referenced this issue in aladdin-add/eslint May 19, 2019
aladdin-add referenced this issue in aladdin-add/eslint May 22, 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
ilyavolodin pushed 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
@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 Nov 22, 2019
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 core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
10 participants