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

Configuration to enable all rules #6240

Closed
mockdeep opened this issue May 22, 2016 · 29 comments
Closed

Configuration to enable all rules #6240

mockdeep opened this issue May 22, 2016 · 29 comments
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 feature This change adds a new feature to ESLint help wanted The team would welcome a contribution from the community for this issue

Comments

@mockdeep
Copy link
Contributor

I would love to have a configuration that enables all rules, that way I can selectively disable them:

extends: ['eslint:all']

This way I can easily be made aware of new rules when I upgrade, and don't have to go hunting through the docs to figure out which ones to add to my configuration file.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label May 22, 2016
@mysticatea mysticatea added core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint 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 23, 2016
@mysticatea
Copy link
Member

Related: #2730.

@mockdeep
Copy link
Contributor Author

Oh, okay. Should I comment there instead? I hacked together a basic script which does it, but it would be great to have something out-of-the-box. Needs some cleanup, but it did the trick:

var _ = require('lodash');

var eslintRules = require('eslint/conf/eslint.json').rules;
var eslintEnabled = _.mapValues(eslintRules, function () { return 'error'; });

var reactRules = require('eslint-plugin-react').rules;
var disabledRules = {'react/jsx-sort-prop-types': true};
var reactEnabled = _.reduce(reactRules, function (result, value, key) {
  const ruleName = `react/${key}`;

  if (disabledRules[ruleName]) { return result; }
  result[ruleName] = 'error';

  return result;
}, {});

var allEnabled = _.extend({}, eslintEnabled, reactEnabled);

module.exports = { rules: allEnabled };

@nzakas
Copy link
Member

nzakas commented May 23, 2016

We can discuss here. The concern with having all rules enabled is that not all of them have defaults that make sense for most people. For instance, people are generally split between double and single quotes, so what would you expect that rule to do when enabled by default? Maybe it doesn't matter, but I'm not sure.

I'm not opposed to the idea, I just want to make sure we have a good idea of people's expectations around this.

@jfmengels
Copy link
Contributor

jfmengels commented May 23, 2016

If you want to familiarize yourself with all the rules by turning them on and seeing what kind of errors it shows in your codebase (and reading their doc file only when needed), you're better off copy-pasting the content of this file (https://github.com/eslint/eslint/blob/master/conf/eslint.json) (though I think it's a bit hidden) and setting everything to error.

If you want to find out about new rules easily, you can read the changelog or use tools like @sarbbottam's https://github.com/sarbbottam/eslint-find-rules.

For the rest, I don't see the point. And I agree with @nzakas that you need defaults for a few of these rules, and it doesn't make much sense to just turn them on like that.

@mockdeep
Copy link
Contributor Author

To me it actually doesn't matter which one is the default. For my own part, I'm happy to go about reconfiguring things to suit my needs, but I want to have a simple way to be made aware of new rules, or rule changes, without having to go hunt them down in the changelog or elsewhere. It's easy to miss a new rule, also, for one reason or another, so unless I go to the pain of re-copying the default configuration and editing it, it's hard to be sure I'm aware of everything.

For me, it's about having as transparent and automatic a process as possible when upgrading to get the new rules enabled. If it's something that I'm already in line with, great! If not, I might make the changes on the fly or add it to the disabled list for now. If I always have to go back to find the new rules that are added or use a separate tool, it's more likely not to get done, and my code suffers for it. Not by any means arguing that it needs to be the default, but it would be nice to have an option to include all of them.

Another side effect of enabling all by default is that I have a much shorter configuration file (50 lines at the moment, including comments and react extension configuration), and instead of documenting all of the rules that are enabled, I have instead documented just the ones that depart from the default. This makes the file a little less overwhelming to look at, at least for me, and maybe a little more friendly to newcomers to a project.

root: true
# have to include `plugin:react/recommended` for some reason to avoid errors
extends: ['./.eslint-all.js', 'plugin:react/recommended']
plugins: [react]
env:
  browser: true
  es6: true
  node: true
globals:
  $: true
ecmaFeatures:
  jsx: true
rules:
  # to enable someday
  'react/forbid-prop-types': [off]
  'react/no-multi-comp': [off]
  'react/no-set-state': [off]
  'react/no-string-refs': [off]
  'react/prefer-es6-class': [off]
  'react/prefer-stateless-function': [off]
  # departures from default
  brace-style: [error, 1tbs, { allowSingleLine: true }]
  func-style: [error, declaration]
  id-length: [error, { exceptions: [_, $] }]
  indent: [error, 2]
  jsx-quotes: [error, prefer-single]
  max-len: [error, 115]
  max-statements-per-line: [error, { max: 2 }]
  max-statements: [error, { max: 14 }]
  no-underscore-dangle: [error, { allowAfterThis: true }]
  object-property-newline: [error, { allowMultiplePropertiesPerLine: true }]
  one-var: [error, never]
  padded-blocks: [error, never]
  prefer-arrow-callback: [error, { allowNamedFunctions: true }]
  quote-props: [error, as-needed, { keywords: true }]
  quotes: [error, single]
  space-before-function-paren: [error, { anonymous: always, named: never }]
  spaced-comment: [error, always, { markers: [=] }]
  'react/jsx-indent': [error, 2]
  'react/jsx-indent-props': [error, 2]
  'react/wrap-multilines': [error, { declaration: false }]
  # disabled
  init-declarations: [off]
  no-inline-comments: [off]
  no-magic-numbers: [off]
  no-ternary: [off]
  prefer-reflect: [off]
  require-jsdoc: [off]
  'react/jsx-handler-names': [off]
  'react/jsx-max-props-per-line': [off]
  'react/jsx-sort-props': [off]

@nzakas
Copy link
Member

nzakas commented May 24, 2016

@mockdeep a significant downside of "eslint:all" would be that every time we added a new rule, that would automatically be enabled for you. In essence, "eslint:all" would be different every time you upgrade ESLint, meaning you'd constantly need to update your config for every upgrade.

Technically, that would be a breaking change every time we add a rule, and I'm not sure that's good for anybody.

@sarbbottam
Copy link

I want to have a simple way to be made aware of new rules, or rule changes, without having to go hunt them down in the changelog or elsewhere

eslint-find-rules can help you in this regard.

For me, it's about having as transparent and automatic a process as possible when upgrading to get the new rules enabled.

let eslint-find-rules -u path/to/eslintconfig run before your lint task.

for example:

// package.json
{
  ...
  "scripts": {
    "prelint": "eslint-find-rules [option] <file> [flag]",
    "lint": "eslint"
  }
  ...
}
...

It would error out, if there are unused rules, or in other words, updated/upgraded eslint, eslint-plugins have added new rules, which are not part of your eslintconfig.

@mockdeep
Copy link
Contributor Author

@nzakas I would actually prefer that. That's the way it works with other tools like rubocop unless you configure it otherwise, so it's a nice convenience as far as I'm concerned. When I'm in the mode of upgrading my tools, chances are I want to be made aware of rule updates and do something or other about them. Reducing the work that I have to do for that is doing me a favor.

Again, I'm not arguing that it should be a default, but opt-in. As far as linters are concerned, I don't actually consider it a breaking change, though I suppose it depends on how you look at it. It changes validation behavior as far as the user is concerned, but the programmer API for interfacing with it is presumably unchanged. Linters are in this weird area, but if it's opt-in behavior then the user is taking responsibility for dealing with those changes.

@sarbbottam That looks pretty handy, but it's sort of the inverse. Instead of having to configure non-default rules, I have to configure all of them, which I don't like so much. I'd rather have all of the rules loaded automatically and just configure what departs from the defaults.

@mynameislau
Copy link

I am totally in favor of this, I wanted to implement every rule the other day and it is very time consuming + your config won't cover any new rule. As a result, my config is only partially complete, since I am procrastinating about it. This is exactly the feature I wish I had.

@nzakas
Copy link
Member

nzakas commented May 24, 2016

if it's opt-in behavior then the user is taking responsibility for dealing with those changes.

Generally, the agreement is that something you opt-in to should continue to work the same way unless there's a major release. However, we can always tell people up front that this is the behavior of "eslint:all" so buyer beware.

Okay, @mockdeep, if you want to submit a pull request, we can review it.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion help wanted The team would welcome a contribution from the community for this issue and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 24, 2016
@mockdeep
Copy link
Contributor Author

Okay, I'll see what I can do. May take a few days, though, so if anybody else wants to take up the banner in the meantime, feel free.

Out of curiosity, do you issue a new major release when rules change under the eslint:recommended extension?

@nzakas
Copy link
Member

nzakas commented May 24, 2016

Out of curiosity, do you issue a new major release when rules change under the eslint:recommended extension?

Yes. Whenever we plan on doing a major release, we update "eslint: recommended".

@mockdeep
Copy link
Contributor Author

Would you recommend implementing as a .js file similar to above? (with the react stuff removed of course). It seems like the cleanest option is ideally to not have to duplicate json files with different settings, but I could do that as well.

@mockdeep
Copy link
Contributor Author

Above being here.

@platinumazure
Copy link
Member

@mockdeep You can do that if you're sure that conf/eslint.json will always have all rules. You might find it easier to just use the fs module to iterate over the rules directory, take each filename (stripping the .js extension), and exporting a config object with every rule name set to "error".

@mockdeep
Copy link
Contributor Author

@platinumazure thanks, I'll give that a shot. I wonder if there is a significant performance difference between handling it programmatically versus having a static json file. A third option, if so, might be to have a separate json file, but a build-time verification that it has all the rules listed in it.

@alberto
Copy link
Member

alberto commented May 25, 2016

I'm not sure this is worth adding to core for the following reasons:

  • There is already a package that gives you that info. (Maybe this could also be created as a plugin instead?)
  • We would make an exception for semver for this one.
  • Are we sure ALL present and future rules have defaults?
  • Are we sure ALL present and future rules can be enabled at the same time without causing conflicts?

@mockdeep
Copy link
Contributor Author

I've mentioned some of this in previous comments, but responding directly:

There is already a package that gives you that info.

The package doesn't really do what I'm asking for here. It tells you about rules you don't have in your config file, whereas I want to just have all rules enabled by default, and simply configure as necessary away from the defaults.

(Maybe this could also be created as a plugin instead?)

It could, I suppose, though it seems like a core configuration concern, so it would be nice not to have to install extra packages just to get eslint configured on a repo. Having it as a plugin would also, I think, limit the ways to implement something like this. If we wanted to switch to a static json file, I'm not sure if there's a clean and cross-version flexible way to handle that as a plugin.

We would make an exception for semver for this one.

As mentioned above, I don't really see it as an exception to semver, but even if you do, it seems like a reasonable exception. To me semver is about communicating expectations to your users. A major version update has a reasonable chance of breaking your application, a minor version is just adding new functionality and shouldn't break the existing. In the case of linters, a minor version update signals to me that there are likely to be new rules, and having an option that reduces the overhead for me to integrate these new rules is to me in line with the intentions of semver. As your user, the expectations are clear to me because I've opted into this behavior.

Are we sure ALL present and future rules have defaults?

It seems that all the current ones do. Even if new rules are added that don't, it'll be easy enough to modify this to accommodate those as well.

Are we sure ALL present and future rules can be enabled at the same time without causing conflicts?

I didn't run into any conflicts that I can think of when integrating this into one of my applications, but as a user I'm happy to take on the responsibility of dealing with that as well. It won't cause actual breakage, I'll just have to decide which one I want to abide by.

@pedrottimark
Copy link
Member

This way I can easily be made aware of new rules when I upgrade, and don't have to go hunting through the docs

Indeed there is a gap in information architecture: rule docs and version blogs have answers only if you know where to ask the questions.

@mockdeep On a separate path from "eslint:all" do you see any value in brainstorming about a counterpart to the rules index page on the ESLint Web site organized by version from new to old?

What information would be useful?

@mockdeep
Copy link
Contributor Author

mockdeep commented Jun 3, 2016

@pedrottimark I can't say that something like that would be particularly useful in my case. Even then it would be more overhead to track down the appropriate docs page and look it over, as opposed to just having them in your face when you upgrade. I like tools that don't allow me to be lazy or forgetful. In a lot of cases, we're already in line with new rules, so they just silently pass and that's great.

I could see there maybe being some docs improvements for people who are happy with the current workflow. Using our process as an example, upgrades on projects vary. Main projects we upgrade every week or two, so the most recent additions are pretty easy to suss out. Others might be a few months between upgrades, so the diff is a lot bigger. In those cases, one thing I can think of is to have a dynamic "new since version" tool of some sort where you could filter just the changes that are relevant to you.

Another thing to keep in mind is that people might be using multiple linters, so at a higher level view it's additionally helpful to be able to get that sort of information in one place. In our case, aside from eslint we're using eslint-plugin-react, rubocop, brakeman, scss-lint, and haml-lint. Even if the docs are super friendly on all of these, I still have to go hunting them down each upgrade.

Kind of tangential, but one thing that could be really helpful is for there to be a link to the docs for a rule when something fails a check. It's great that they have urls corresponding to the names, since I ended up just typing them instead.

@platinumazure
Copy link
Member

@mockdeep Would your needs be better met by, rather than pulling in all rules and linting them via configuration, if you could instead use a CLI flag to warn on rules not explicitly configured? That way auto-fix won't potentially make risky changes to your code but you would get an error for not having a configuration explicitly set for a new rule.

@mockdeep
Copy link
Contributor Author

mockdeep commented Jun 3, 2016

@platinumazure I'm partial to not having to configure every single rule. I mentioned before, but it's nice to have my config file represent only the things that depart from the default, as opposed to having to have every single rule configured.

You solution could be handy in terms of making me more aware of new rules, but a lot of times when rules are added my codebase is already in line with the defaults, so with eslint:all it just transparently starts being enforced, no extra work on my part. I could see it being a useful feature for people who prefer to have everything explicitly configured, though, similar to @sarbbottam's solution above.

I'm not actually too concerned about auto-fix breaking code for us, being that we have multiple layers of safety to prevent it from being a problem in house. We've got solid tests, code review, and just make a habit of keeping commits small and easy to review with confidence. If auto-fix changes too much we'll just git checkout and break it down into smaller chunks by disabling rules or ignoring subsets of files. That's obviously not the case for everybody, so warnings might be appropriate.

Most of the auto-fix stuff is pretty small, though, and seems unlikely to cause breakage. Spacing and minor style adjustments. Maybe more risky if you have syntax errors in your code already.

@pedrottimark
Copy link
Member

one thing that could be really helpful is for there to be a link to the docs for a rule when something fails a check

@mockdeep Some integrations provide this. You might be pleasantly surprised.

Atom editor:
6240 atom

http://eslint.org/docs/user-guide/formatters/html-formatter-example
6240 formatter

@mockdeep
Copy link
Contributor Author

mockdeep commented Jun 3, 2016

Ah, nice. I usually just run it at the command line, though. Is there a formatter there that does something similar?

@pedrottimark
Copy link
Member

CLI has option for formatter: http://eslint.org/docs/user-guide/command-line-interface#f---format

If you see a solid way to do it, seems we should consider adding as specific example to that section.

@pedrottimark
Copy link
Member

At this time on a Friday evening, I had to look at that anchor 3 times: is it a grimace 😬 or a grin 😁

@knownasilya
Copy link

Am I doing this correctly? https://github.com/knownasilya/eslint-plugin-doc-code-blocks#configuration it says the rule doesn't exist..

@platinumazure
Copy link
Member

@knownasilya You shouldn't prefix core rules with doc-code-blocks/. Prefixing a rule with a plugin name means ESLint will assume it's a plugin rule. Your example should just use "quotes".

@knownasilya
Copy link

@platinumazure thanks, I'll use separate configs, as you suggested on gitter.

@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
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 feature This change adds a new feature to ESLint help wanted The team would welcome a contribution from the community for this issue
Projects
None yet
Development

No branches or pull requests