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

Cannot overwrite rule options from sharable config #6144

Closed
danny-andrews opened this issue May 11, 2016 · 12 comments
Closed

Cannot overwrite rule options from sharable config #6144

danny-andrews opened this issue May 11, 2016 · 12 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@danny-andrews
Copy link
Contributor

danny-andrews commented May 11, 2016

What version of ESLint are you using?
v2.9.0

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

Please show your full configuration:
my eslint-es5.js:

{
  extends: ['airbnb-base/legacy'],
  rules: {
    'eqeqeq': 'error'
  }
}

Relevant part of airbnb-base/legacy.js:

{
  rules: {
    'eqeqeq': ['error', 'allow-null']
  }
}

What did you expect to happen?
I expected there to be a way to overwrite this 'allow-null' exception because I want to enforce eqeqeq at all times.

What actually happened? Please include the actual, raw output from ESLint.
I am still able to do bool == null.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label May 11, 2016
@mysticatea mysticatea added core Relates to ESLint's core APIs and features breaking This change is backwards-incompatible 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 May 13, 2016
@mysticatea
Copy link
Member

Thank you for this issue.

I think this is an intentional behavior.
If this behavior is confusing, we should evaluate the value of the breaking change.

@ilyavolodin
Copy link
Member

I think we should just update documentation to make it clear that configs are deep-merged.

@danny-andrews
Copy link
Contributor Author

danny-andrews commented May 13, 2016

That doesn't solve these types of issues where you can't override behavior from extended configs. A non-breaking change we could make is to allow a predefined string to be passed as the second argument of a rule which tells eslint to use the rule defaults. Something like,

{
  "rules": {
    "eqeqeq": ["error", "use-defaults"]
  }
}

This would disable merging of rule options for this rule and use the defaults defined for this rule.

@alberto
Copy link
Member

alberto commented May 13, 2016

I think there should be an explicit default option for eqeqeq

@nzakas nzakas added rule Relates to ESLint's core rules and removed core Relates to ESLint's core APIs and features labels May 15, 2016
@danny-andrews
Copy link
Contributor Author

danny-andrews commented May 17, 2016

@alberto I feel like that is a bit of a band-aid solution, as this is an issue for other rules as well.

@alberto
Copy link
Member

alberto commented May 17, 2016

Then we should add options for those as well.

@danny-andrews
Copy link
Contributor Author

Fair enough. Just seems error prone to have to add those options on a rule-by-rule basis.

@danny-andrews
Copy link
Contributor Author

Any updates on this?

@platinumazure
Copy link
Member

I'm not sure I agree with the logic that (in the OP's example) allow-null option should remain. I believe we should have configuration be all-or-nothing at the rule level-- if a rule is specified in derived config, it is assumed to be a full configuration that completely replaces the configuration specified in base config file.

Note that this still allows for the most common cases (derived config turns off rule enabled in base config, derived config turns on rule disabled in base config) to continue to work as they always have. In the case of derived config turning a rule on, the derived config must already specify a full and valid configuration.

In the case described by OP, we have an example of configuration being implicit in the derived configuration file. That strikes me as violating the principle of least surprise and I think we should change it, even if it's a breaking change.

@danny-andrews
Copy link
Contributor Author

danny-andrews commented Jun 7, 2016

I agree with @platinumazure on this. I found it very surprising to inherit options from parent configs for rules I overrode in child configs. But at least if we were given a explicit default option, we could work around this. As it is, you can get stuck with certain rule options from parent configs and there is no way to override them.

@alberto
Copy link
Member

alberto commented Jun 8, 2016

@danny-andrews I added an option for eqeqeq. Have you found any other rule without an explicit default?

@alberto
Copy link
Member

alberto commented Jul 8, 2016

Closing in favor of #6361

@alberto alberto closed this as completed Jul 8, 2016
nzakas pushed a commit that referenced this issue Nov 8, 2016
* Update docs to explain config option merging

There was a lot of [confusion](#6361) generated by the merging behavior of rules inherited from base configs, so I thought I would clear that up here. There still may be [scenarios](#6144) which arise where options cannot be overridden from base configs, but we'll deal with those on a per-case basis.

* Use 4 spaces, not 2

* Use string notation, instead of numeric

* Fix level
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@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 Feb 6, 2018
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 breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

7 participants