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

New: Add eslint:all option (fixes #6240) #6248

Merged
merged 1 commit into from Jun 8, 2016
Merged

New: Add eslint:all option (fixes #6240) #6248

merged 1 commit into from Jun 8, 2016

Conversation

mockdeep
Copy link
Contributor

This adds a new option to allow a user to extend eslint:all in order
to enable all rules by default.

@eslintbot
Copy link

Thanks for the pull request, @mockdeep! I took a look to make sure it's ready for merging and found some changes are needed:

  • Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @alberto, @nzakas and @gyandeeps to be potential reviewers

@eslintbot
Copy link

LGTM

@mockdeep mockdeep changed the title New: Add eslint:all option (#6240) New: Add eslint:all option (refs #6240) May 25, 2016
@mockdeep
Copy link
Contributor Author

I was looking for a good place to update the docs around this, but wasn't sure if any of the docs in the repo made sense. Seems like more of an advanced configuration thing.

@@ -0,0 +1,13 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the same format as other files: http://eslint.org/docs/developer-guide/code-conventions#file-format

@nzakas
Copy link
Member

nzakas commented May 25, 2016

@mockdeep can you update your commit message to say "fixes" instead of "refs"?

@pedrottimark any idea where docs for this should go?

var fs = require("fs");
var path = require("path");

var ruleFiles = fs.readdirSync(path.resolve(__dirname, "../lib/rules"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should lazyload this since not everyone is going to be using eslint:all but they would still endup paying the price for a whole directory scan at load time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this only get loaded if they have it configured?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. sorry I dint see the require on the other end.

@eslintbot
Copy link

LGTM

@mockdeep mockdeep changed the title New: Add eslint:all option (refs #6240) New: Add eslint:all option (fixes #6240) May 26, 2016
@mockdeep
Copy link
Contributor Author

added format comments and updated commit message

@kaicataldo
Copy link
Member

kaicataldo commented May 30, 2016

I think it makes sense to mention this on the rules page underneath the section that goes over "extends": "eslint:recommended", even if we also mention it elsewhere

@nzakas
Copy link
Member

nzakas commented Jun 1, 2016

@kaicataldo we could briefly mention it, but we also need a big warning about it's use, and I don't think jamming that at the top of the rules page makes sense.

Ping @pedrottimark

@pedrottimark
Copy link
Member

pedrottimark commented Jun 1, 2016

Sorry, my bad to overlook this one. Agree not on the rules index page.

@pedrottimark
Copy link
Member

pedrottimark commented Jun 1, 2016

Here is a rough draft for comment first priority about the place and structure:

Extending Configuration Files

If you want to extend a specific configuration file, you can use the extends property and specify:

  • "eslint:recommended" enables a subset of core rules that report common problems, which have a check mark (recommended) on the rules page
  • "eslint:all" enables all core rules in the currently installed version of ESLint AND SO ON I WILL WRITE A FIRST DRAFT IN A SEPARATE COMMENT
  • a shareable configuration package (for example, "eslint-config-myrules")
  • a configuration from a plugin that provides configurations (for example, "plugin:eslint-plugin-myplugin/myConfig")
  • the path to a configuration file (for example, "./node_modules/coding-standard/.eslintrc")

//------------------------------------------------------------------------------

var ruleFiles = fs.readdirSync(path.resolve(__dirname, "../lib/rules"));
var enabledRules = lodash.reduce(ruleFiles, function(result, filename) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little curious about differences from ruleFiles.reduce(function(result, filename) {.
Why lodash is needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good call. I'll change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I had some changes locally where I was trying to remove the lodash dependency using Object.keys. I had forgotten reduce was available on arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to remove lodash dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍

This adds a new option to allow a user to extend `eslint:all` in order
to enable all rules by default.
@eslintbot
Copy link

LGTM

@pedrottimark
Copy link
Member

pedrottimark commented Jun 2, 2016

Thinking what description will say as much as possible in as few words as possible.

@mockdeep Just thinking through the phrase “I want to be made aware of rule updates” do you mean when a new rule reports problems, then you read about it and decide one of the following:

  1. agree that the problems are problems, fix them, and keep the rule enabled with default options
  2. disagree with the problems, but decide that the rule is relevant, so specify non-default options
  3. disagree with the problems, and decide that the rule is not relevant, so turn it off

What are your thoughts about the safety of using "eslint:all" with the --fix option? It seems like a new fixable rule can make potentially disagreeable changes, but even if you notice those changes in a diff, the rule does not report problems to take credit or blame for who done it. [Please don’t take the statement as sarcasm. In technical writing, definite statements are steps toward correct clear conclusions; even if the first step is definitely wrong.]

@nzakas I am reading and rereading your comments in #6240 and would be glad for guidance about anything you consider definitely must-say and any max or min number of sentences to say it?

Do you think it is worth making an explicit contrast: which rules are recommended changes only at major versions, but which rules are in all can change at any minor version.

Thinking about major versions somehow sent my mind back to the future: will deprecated rules still be included in all?

@pedrottimark
Copy link
Member

pedrottimark commented Jun 2, 2016

@nzakas P.S. If you think that the content size target is more than a few sentences in a bullet point, what do you think about applying the new structure for rule options in Configuring ESLint:

Extending Configuration Files is already a level-2 heading like Options

EDIT: Provide a bulleted list summary of values for extend as mentioned in #6248 (comment)

Put the examples and any extended description under level-3 headings as we do with options.

@mockdeep
Copy link
Contributor Author

mockdeep commented Jun 2, 2016

@pedrottimark

then you read about it and decide one of the following:

  1. agree that the problems are problems, fix them, and keep the rule enabled with default options
  2. disagree with the problems, but decide that the rule is relevant, so specify non-default options
  3. disagree with the problems, and decide that the rule is not relevant, so turn it off

Yep, that sounds reasonable, with the possibly addition of a fourth:

"4. agree that the problems are problems, but fixing would be non-trivial or bloat the diff, so turn it off with a note to fix later"

but even if you notice those changes in a diff, the rule does not report problems to take credit or blame for who done it

I don't fully understand what you're saying in this bit. Are you saying that the rule change won't show up in the git history because it's not being added to a rule file? That's not a huge bother really as far as we're concerned. The commit message of the change should have enough detail to let people know where it came from.

I generally have an eye towards small commits, so when upgrading if it's only a handful of changes I will go ahead and do them, but if there are a bunch I might actually disable the rule and then re-enable it on a followup commit to keep the history more clear. I will first typically run eslint without the --fix option and depending on the outcome might make fixes manually, or if it's a sweeping change run the --fix option.

@pedrottimark
Copy link
Member

pedrottimark commented Jun 2, 2016

@mockdeep Sorry I was’t so clear the first attempt. If you lint first without --fix that is a way to solve my concern because you become aware of a new rule from the problems that it reports.

I’m thinking about how likely and serious a risk to people who are less careful and do-it-yourself, who might learn of --fix and "eslint:all" from different blog articles, but not understand enough about how they interact to be able to identify a new fixable rule as the cause for unexpected changes EDIT if they lint with --fix all the time.

Because I am not strong at devops, I can relate to recent cautions about risk of using “boilerplate” configurations with only shallow understanding of the options for the various tools.

@mockdeep
Copy link
Contributor Author

mockdeep commented Jun 2, 2016

@pedrottimark Ah, I see. Well it seems like that would be less of a problem if you're simply upgrading, since you're aware of what you're up to and the changes are likely to be much smaller. But even on a new install if you enable eslint:recommended or just copy a config file from somewhere on the internet, if you run with --fix you're going to have similar risks without the eslint:all option.

@platinumazure
Copy link
Member

@mockdeep Bearing in mind that the risk with either eslint:recommended or eslint:all is when a new rule comes in with a fixer that might do something you don't like until you figure it out and configure around it... The difference between those two extensions is that eslint:recommended changes only on ESLint major version bumps (and according to semver, you really should expect all hell to break loose on major version upgrades anyway), whereas eslint:all gets new and potentially risky rules on minor bumps. Most people configure ESLint with a minor-friendly version range (such as ^2.11.0), so they will run into unexpected behavior almost every time they npm install.

I'm not trying to torpedo the proposal-- we can certainly add warnings a-plenty in documentation and we can certainly encourage people to pin their ESLint versions when using eslint:all. But there is a material difference in level of risk between eslint:recommended and eslint:all.

@pedrottimark
Copy link
Member

Yes, agree that both are similar for new installation. Forgot to say that I am not arguing against the feature, but thinking about what information people need to know and where to put it so they find it.

@mockdeep
Copy link
Contributor Author

mockdeep commented Jun 2, 2016

Makes sense. I'm all for having clear documentation to make sure expectations are clear.

@nzakas
Copy link
Member

nzakas commented Jun 3, 2016

From my point t of view, we need to communicate:

  1. This is a "void your warranty" configuration. It will be breaking with every new release that adds a rule and you accept the overhead of updating your config.
  2. We explicitly do not recommend this configuration for production use. It is best used for exploring ESLint rules before deciding on a final configuration.
  3. Default options for rules do not indicate endorsement of particular settings -- we just want to make sure all rules do something by default.

@pedrottimark
Copy link
Member

Am working on a pull request with changes to Extending Configuation Files in configuring.md

@nzakas Should it have commit tag Docs and ref this pull request?

@nzakas
Copy link
Member

nzakas commented Jun 6, 2016

@pedrottimark should ref #6240

@nzakas
Copy link
Member

nzakas commented Jun 8, 2016

The code looks good, so I'm going to merge. We can work on the documentation next.

@nzakas nzakas merged commit f804397 into eslint:master Jun 8, 2016
@mockdeep mockdeep deleted the rf-rule_them_all branch June 14, 2016 15:47
@neoadventist
Copy link

@nzakas , @mockdeep any idea on how to do this with the cli? I'm trying to have the rules enabled in vim that I can use on any file without having to specify a config file each time I want to work on a new project.

@platinumazure
Copy link
Member

platinumazure commented Jan 4, 2017 via email

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet