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

Recommended config has disabled defaults #4809

Closed
IanVS opened this Issue Dec 26, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@IanVS
Copy link
Member

IanVS commented Dec 26, 2015

This caused me to scratch my head for a while. The eslint:recommended configuration has some rules that are disabled but contain an option, like:

{
    "complexity": [0, 11]
}

Normally this would have no effect, but due to the way configs are merged, if my own config has:

{
    "complexity": [2]
}

The end result is "complexity": [2, 11], which is not what I was expecting. I don't believe this was intentional, and is an artifact of having been created with a severity and value, and then disabled without removing the value (50a4e46).

I think in most cases, the options we specify happen to be the default values, so the effect would be masked, but complexity happens to not have a default value (#4808), so I couldn't figure out why I was getting errors.

I propose to remove all of the option configurations from disabled rules within our recommended config, to prevent other surprises people may encounter.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Dec 26, 2015

@IanVS Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Dec 26, 2015

@IanVS IanVS added bug core and removed triage labels Dec 26, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 26, 2015

Agree

@nzakas nzakas added evaluating and removed accepted bug labels Dec 26, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 26, 2015

This was intentional so there was always one place to go to see all default options, so not a bug. It's also a fallback to make sure that default values are provided for all rules in case someone just decided to enable without specifying other options (as you did), otherwise, an enabled rule might do nothing.

I'm not opposed to removing these defaults, but I'd like a proposal for how to achieve what this is currently doing.

@IanVS

This comment has been minimized.

Copy link
Member Author

IanVS commented Dec 27, 2015

I propose making sure that all rules have their own defaults (e.g. #4808), and then removing them from the recommended config, in the interest of clarity. Otherwise, some rules will have defaults when extending from recommended, but not have defaults when not extending it. I see this as being very confusing and unexpected (it was for me!). I can make a PR if you agree to this approach.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 27, 2015

Sounds good.

@IanVS

This comment has been minimized.

Copy link
Member Author

IanVS commented Dec 31, 2015

I notice that no-multiple-empty-lines has a config of [0, {"max": 2}] in eslint.json, but the default for the rule does not allow more than 1 empty line. Anyone have a problem with removing this conflicting default from eslint.json? It will only affect people who were extending from eslint:recommended, and turned the rule on without an options object.

As a seperate note (mostly to myself, so I remember to address it), the following rules have undocumented defaults:

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 1, 2016

Fine by me. Can you add this info to the migrating to 2.0.0 doc?

@IanVS

This comment has been minimized.

Copy link
Member Author

IanVS commented Jan 6, 2016

@nzakas, do you accept my proposals in #4833, #4834, and #4835? They need to be addressed before progress can be made on this issue.

@IanVS IanVS added the blocked label Jan 6, 2016

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 6, 2016

Yeah, go for it. Somehow I missed those.

@IanVS

This comment has been minimized.

Copy link
Member Author

IanVS commented Jan 7, 2016

I was wrong on two of them, they had sneaky, undocumented defaults. Now the only blocker is #4868.

IanVS added a commit that referenced this issue Jan 7, 2016

IanVS added a commit that referenced this issue Jan 7, 2016

IanVS added a commit that referenced this issue Jan 7, 2016

@IanVS IanVS removed the blocked label Jan 7, 2016

@gyandeeps gyandeeps added the breaking label Jan 7, 2016

@gyandeeps gyandeeps added this to the v2.0.0 milestone Jan 7, 2016

@IanVS IanVS closed this in e2f2b66 Jan 7, 2016

nzakas added a commit that referenced this issue Jan 7, 2016

Merge pull request #4871 from eslint/issue4809
Breaking: Remove defaults from `eslint:recommended` (fixes #4809)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.