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

overrides settings in shareable configs overwrite extender's setting #11510

Closed
mysticatea opened this issue Mar 15, 2019 · 2 comments
Closed

overrides settings in shareable configs overwrite extender's setting #11510

mysticatea opened this issue Mar 15, 2019 · 2 comments

Comments

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Mar 15, 2019

This issue came from eslint/rfcs#13.

Currently, overrides is applied after all overrides properties are merged. This means that the overrides in a shareable config priors to the setting in your .eslintrc file.

extends:
    - foo
rules:
    eqeqeq: error # silently ignored if the "foo" has the "eqeqeq" setting in "overrides".

After this proposal, the setting in your .eslintrc file priors to the setting in shareable configs always. This is a breaking change, but I think this is a bug fix.

https://github.com/eslint/rfcs/blob/42cd563d23eaaad5e9206d8ca7463aeb7dc75a69/designs/2019-eslintrc-improvements/README.md#%EF%B8%8F-fix-a-surprised-behavior-of-overrides

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

# extendee.yml
overrides:
  - files: "*.js"
    rules:
      no-console: error
# extender.yml
extends: "./extendee.yml"
rules:
  no-console: off
echo "console.log()" | eslint --stdin --stdin-filename a.js --no-eslintrc --config extender.yml

What did you expect to happen?

No errors because extender.yml disabled no-console rule.

What actually happened? Please include the actual, raw output from ESLint.

C:\Users\t-nagashima.AD\dev\eslint\a.js
  1:1  error  Unexpected console statement  no-console

✖ 1 problem (1 error, 0 warnings)
@mysticatea mysticatea self-assigned this Mar 15, 2019
@mysticatea mysticatea added this to Accepted, ready to implement in v6.0.0 Mar 15, 2019
@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented Mar 15, 2019

This issue has been accepted, but it's not feasible to implement under the current architecture without dropping config cache functionality. I have solved this bug by changing the internal structure of configs to an array. How much am I allowed to take the implementation out from the PoC https://github.com/eslint/eslint/tree/proof-of-concept/config-array-in-eslintrc/lib/_lookup?

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Mar 15, 2019

I had assumed it would be implemented along with the config refactoring, but without adding the other features from the config refactoring (e.g. replacing --ext).

mysticatea added a commit that referenced this issue Mar 22, 2019
- Passed tests in tests/lib/cli-engine.js
mysticatea added a commit that referenced this issue Mar 24, 2019
- Passed tests in tests/lib/cli-engine.js
mysticatea added a commit that referenced this issue Mar 24, 2019
@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 Mar 26, 2019
mysticatea added a commit that referenced this issue Mar 27, 2019
@mysticatea mysticatea moved this from Implemented, pending review to Ready to merge in v6.0.0 Apr 8, 2019
@mysticatea mysticatea moved this from Ready to merge to Implemented, pending review in v6.0.0 Apr 8, 2019
mysticatea added a commit that referenced this issue Apr 9, 2019
v6.0.0 automation moved this from Implemented, pending review to Done May 10, 2019
mysticatea added a commit that referenced this issue May 10, 2019
… (#11546)
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 7, 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
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants