eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
24.44k stars 4.41k forks source link

Make --reset the default behavior, remove flag #2100

Closed mickaeltr closed 8 years ago

mickaeltr commented 9 years ago

Hello,

I have the following use case:

So what I first did is call eslint --reset with:

The problem is that the --reset flag prevents from applying the default eslint rules on the specific folders.

I see 3 options:

  1. Call eslint twice: one time with --reset, another time on the specific folder with --reset
  2. Disable all rules manually in the root .eslintrc
  3. Enable all rules manually in the specific folder .eslintrc

Those options are not really desirable, so I am thinking that it would be nice to be able to reset all rules in the .eslintrc file, so it could be overriden by more specific configuration files below.

What do you think?

nzakas commented 9 years ago

I'm not sure how feasible it is to introduce reset into the middle of a configuration hierarchy. Can you explain more about why you need this? Why would you want reset to apply to only some files?

mickaeltr commented 9 years ago

My use case is the following:

If I disable the default rules at the root with --reset, these default rules won't apply to my specific folder.

So my best solution so far is to run twice eslint:

Thus, the idea is to be able to reset the rules from the configuration file, not only from the command line. I would then end up with something like:

nzakas commented 9 years ago

You just repeated your original description. I'm asking why you want to do this? I'd like to understand what situation you're dealing with that led you to want to do this because this is the first time sometime has presented such a case and I need to better understand the rationale to see if it's worth introducing a change or if there's another option.

mickaeltr commented 9 years ago

I work on a legacy project and we want to gradually improve it. We want to apply a minimal set of very important rules to the whole project (at the root). We want to apply stricter rules in newly created modules (in specific folders).

nzakas commented 9 years ago

Okay, so it's not that you want to turn off all rules in a particular directory, it's that you want to enable all rules in a particular directory. I think this might be better served by config extension (as described in https://github.com/eslint/eslint/issues/1637).

puzrin commented 9 years ago

Supporting --reset from config is useful to simplify eslint calls from different programs. For example, many editors have plugins to run linters. They catch configs automatically, but need explicit directive for --reset.

NB. it's enougth to have such --reset support only for top level config.

mickaeltr commented 9 years ago

My first use case was to apply a single rule only (no-console). I first wrote a big .eslintrc file disabling all other default rules. Then I figured out that the combination of .estlintrc {"rules": {"no-console": 2}} + --reset option would be simpler.

Although, I would strongly prefer a self sufficient configuration file.

nzakas commented 9 years ago

@mickaeltr you can already create a self sufficient config file, just use --reset and then put in exactly what you want in your config file.

mickaeltr commented 9 years ago

Oh yes, I figured it out. :)

By self sufficient, I meant a configuration file that does not need any specific option (such as --reset) to work as required. As @puzrin mentionned, it would facilitation the use of eslint from different programs (command line, code editors).

pesla commented 9 years ago

I would like to be able to reset all rules from .eslintrc as well. I'm using JSCS to run code style checks and want ESLint to perform some tasks that JSCS can't. It's annoying to have ESLint perform the same tasks as JSCS just because these rules are turned on by default. Running ESLint with the --reset flag isn't an option, as these processes are run from within my IDE without support for setting flags or whatsoever.

Some out there are actually creating ESLint Resets to have some control, but this looks like maintenance hell.

Anyway +1 for the suggestion of @mickaeltr.

nzakas commented 9 years ago

The problem is that configs cascade, so resetting in one config could affect others. We apply the reset before the first config is read, and then use each subsequent config to override those settings. If we allow reset in a config, then at any point all previous configs will be blown away. This is why it must be an option on the command line - it would completely break config cascade.

puzrin commented 9 years ago

reset does not need cascading. It's needed in root config only.

nzakas commented 9 years ago

Of course, that's why it's a command line option. :) Saying that just one option only applies when the file is in a specific place is confusing. We don't even really have the concept of a root, since we keep searching up for .eslintrc files.

puzrin commented 9 years ago

Saying that just one option only applies when the file is in a specific place is confusing.

Yes, that's not good, but better than nothing. I think, root location is not diificult to formalize. That's topmost file from cwd or explicitly defined in command line.

pesla commented 9 years ago

In the meantime I have "fixed" our issue by using ESLint as our primary linter using a config file with all rules explicitly set to 0, 1, or 2. JSCS comes in to check some additional stuff (and doesn't use defaults that cause side effects).

I see the complexity of the issue, BTW. At least there is some demand for the ability to reset defaults to off. I'm pretty sure that there are enough users that would benefit to justify a (new) feature to do so.

Anyway, thanks for your hard work in ESLint, awesome tool!

nzakas commented 9 years ago

Yup, I hear the request, I'm just not sure there's a rational way to implement.

rlidwka commented 9 years ago

In the meantime I have "fixed" our issue by using ESLint as our primary linter using a config file with all rules explicitly set to 0, 1, or 2.

I tried the same approach for a while. But it turned out to be too painful to change config after each eslint update (it usually has new rules to turn off).

Frankly, I think --reset should be the default. Maybe add presets on top of it for whoever wants them.

nzakas commented 9 years ago

That's an interesting idea. My concern is that there's a belief that something should work "out of the box" in some meaningful way. If we turn off all rules from the start, we lose that completely and people are left needing to manually configure every single rule they want. While that would be fine for power users, it's ominous for everyone else.

So, I'm not opposed to having --reset be the default at some future point, but we'd definitely need some way to get people bootstrapped before we could make such a drastic change.

puzrin commented 9 years ago

but we'd definitely need some way to get people bootstrapped before we could make such a drastic change.

  • built-in option to create config or use some defaults.
  • warning when no rules, with recommendation what to do.
nzakas commented 9 years ago

Okay, now that we are working on --init, I think we can go ahead and make --reset the default behavior. What this means:

ilyavolodin commented 9 years ago

I think I somehow missed this discussion completely. But I'm not sure I buy the idea of reset by default. There are probably over 200-300 different projects that rely on eslint already. With this change, each and every one of them will have to update their .eslintrc file in order to enable just a few people who are having a problem currently. I would much rather see a reset option inside .eslintrc file that will reset any upstream configs all together.

pascalduez commented 9 years ago

Indeed, this sounds a bit concerning. Given the high number of projects we are using ESLint on, what would that imply for existing implementations ?

One of the base feature I got to love about ESLint is this ability to have it working out of the box, and inherit all default rules, just overriding the needed ones (which turns out to be few). That way we keep our .eslintrc small.

If it's just about adding an "extends": "default" in existing config files then that's completely fine :-)

puzrin commented 9 years ago

Since --reset change is breaking, it's a good chance to rename .eslintrc -> .eslintrc.yml too.

pascalduez commented 9 years ago

it's a good chance to rename .eslintrc -> .eslintrc.yml too

Why would that be needed ? I'm curious because that's what we do in SassDoc as well with a .sassdocrc supporting both JSON and YAML through the great js-yaml ;-)

puzrin commented 9 years ago

Why would that be needed ?

highlighting in editors, that was discussed but rejected

gyandeeps commented 9 years ago

I just saw this discussion but I am little reluctant with the idea of making reset by default. As many projects (including 300 people with different projects in my company) rely on eslint suggested config by default. And then they add rules on top of that. But this would break all of those projects and also other projects who use eslint now.

Does that mean that if I don't have any eslint config file and if the reset is done then eslint would not throw any errors/warning? That lets me down that its not very useful right out of the box.

Sometimes people dont know what to check and not check so as of now eslint gives them a nice start too.

gcochard commented 9 years ago

Seems like this is a fairly controversial feature due to the breaking nature.

would this be best introduced in 1.0.0?

rlidwka commented 9 years ago

Sometimes people dont know what to check and not check so as of now eslint gives them a nice start too.

Well the thing is: eslint does not give a nice start. By default it enforces its own codestyle, which is very strict so most people will likely find at least one rule they don't agree with.

Now what do you do when eslint default rules don't work well with your code style? Well there are two approaches to this problem:

  1. You can disable those rules.
  2. You can use --reset and explicitly enable rules you want to use.

First option is good when you need to disable just one or two rules. But if you need to disable more (and you usually do since eslint has hundreds of them), you'll end up in trouble (I'll repeat my points in https://github.com/iojs/io.js/pull/1539#issuecomment-98138376):

So I almost always end up with a --reset flag enabled. If that's the case, why don't make it the default?

nzakas commented 9 years ago

Here's the thing: this isn't just affecting a few people. There are many who are configuring every single rule in order to override the defaults to 0. That's a pretty lousy experience as we continue making changes.

Further, continuing to have rules on by default means that shareable configs only work correctly with --reset, which is also a lousy experience.

For new projects, we will have --init that can setup a config for you, including the ESLint recommendations, so the out-of-the-box experience is just as good (I'd even argue it's better).

For existing projects, we can provide something like @pascalduez mentions so you can use extends to quickly pull back in the recommendations.

So, I think the end result means there is some work for existing projects to upgrade, but the amount of work is pretty small and likely automatable.

The experience of @rlidwka is exactly what I'd like to fix. After 1.0.0, we will not be removing rules anymore. Rules may be deprecated, but not removed, so that helps part of the pain. The rest is just having --reset be the default.

@gcochard this is slotted for the 1.0.0 milestone.

@gyandeeps without a config, we would still report syntax errors.

And keep in mind, we have several breaking changes being made in 1.0.0, so it's not like projects wouldn't have to modify their configs already.

I hope this explains my thinking a bit better. I really do believe that making this change will be a big improvement for both the projects that use ESLint and the ESLint ecosystem as a whole. Please feel free to keep bringing up concerns, I want to make sure people are on board and that the upgrade experience is as smooth as possible.

ilyavolodin commented 9 years ago

Why can't we just add "reset": true to the config, that will basically short-circuit config inheritance at that level? It feels like it should solve the problem for the people who are having issues without making everyone who is currently relying on ESLint re-write their configs. We can also add support for "extends":"base" that will bring back eslint defaults, if necessary. A few breaking changes that are already committed into 1.0.0 will only affect people who are currently using modified rules, not everyone. This change will affect pretty much every user of ESLint.

nzakas commented 9 years ago

I've already explained in this thread why I dont think reset: true in config files is a viable solution.

Again, I'm sensitive to upgrade paths, I just see an overwhelming amount of evidence that reset by default makes more sense than continuing to ask people to manually reset.

IanVS commented 9 years ago

As someone who recently began using ESLint, I will say that I found it a bit strange that defaults like double quotes were included automatically, when the about page says:

Rules are "agenda free" - ESLint does not promote any particular coding style

Seems to me that as long as there is a good upgrade path and bootstrapping process, making the changes @nzakas is suggesting will adhere more closely to the principle of least astonishment.

IanVS commented 9 years ago

Add a question to --init "Do you want to use the rules ESLint recommends?" Of yes, then use eslint.json as the base for the created config

How would this look? Would the rules in eslint.json be copied wholesale into the newly created .eslintrc? I suppose that wouldn't work because it would fall out of sync as new rules are added.

This brings up a larger issue of rule changes between versions. Maybe the best option is to add a command like eslint --upgrade which would update to the latest version of ESLint, and then ask the user whether or not to enable each new rule that has been added (or enable all/none).

Just throwing ideas out, apologies if I wandered too far from the issue at hand, but I think upgrades are a related subject and the behavior will change if eslint.json is no longer used by default.

nzakas commented 9 years ago

@ianvs we'd just use eslint.json as a base and add your preferences on top. The way to keep in sync with ESLint-recommended rules would be using something like extends: default in your .eslintrc file (probably not that exact syntax, but something similar).

I think upgrading between versions is a completely separate topic. Feel free to open a new issue about that.

ilyavolodin commented 9 years ago

@nzakas I read your explanation, I'm just not sure I fully understand it. I don't think it wouldn't be hard to change the order of operations in a way we cascade configs. Start with the closest config you can find and then do backwards merge with config files in parent directories. Do a merge with base config last. This way, whenever you run into reset flag, you just stop going up the chain. Maybe I'm missing something, but it feels like a pretty easy change, and I think it would solve all of the complains that we've seen so far (other then agenda free, but eslint is agenda free, since you can disable anything). P.S. I don't want to drag out the discussion any longer then necessary, I just don't think I agree with the arguments provided above. In my opinion opt-out is better then opt-in for this particular case.

nzakas commented 9 years ago

I don't think what you've described is a small change, nor is it obvious what reset means in any given config file. Remember, the config file closest to the file being linted takes highest priority, so I don't know that it's clear what reset would mean in the middle of a config hierarchy. And how would you know if a config file was relying on reset from something else to work correctly? I can only see situations where adding such a flag creates confusion.

Also consider the shareable configs feature...does that mean every time someone wants to use a shared config, they also must include reset: true because otherwise they'll also be getting the ESLint defaults?

Compare that situation to one where we say there are no rules configured by default, so your config file is WYSIWYG.

I do hear the concern that this is a major change, and it makes a not-exactly-linear upgrade path for existing projects. My belief is that this one-time pain will ultimately be a net positive for the project and its users. I think we can get over this one-time pain fairly easily via extends, and that's just as easy a change as changing a rule that is being removed and will otherwise cause an error.

For instance, if we tell people that all they must do is add this line to the config:

extends: "eslint:recommended"

Is there a still a concern about upgrade path?

ilyavolodin commented 9 years ago

I'm starting to feel bad about dragging this issue out, but I do feel it's the biggest breaking change we ever considered. Here's the list of my arguments against it:

  1. This is a silent change. If I started a new project, and I like all of my quotes to be double, but didn't specify that in my .eslintrc file because I get it directly from ESLint base, once this update hits, unless I read release notes (which not everyone does, GA says that each blog entry gets no more then 1000 visitors for the entire life of the site, and npm says that eslint was downloaded 9180 times just today!) I will have no idea that quotes rule is now disabled, and my code is not being checked for it anymore.
  2. Simple search for ".eslintrc" in github alone returns 5000+ results. That means that 5000+ people will have to go and modify their .eslintrc files once this update hits.
  3. Please keep in mind internet phenomenon of vocal minority vs. silent majority. ESLint has been downloaded 185165 time in this month alone, yet I've only seen 5-10 people complaining about defaults. You are basically making majority suffer in order to please very small minority.
  4. As far as I'm aware, every linter out there comes with the set of defaults, so people are used to it.
  5. Having reset in .eslintrc file might be hard to understand, but I think it's a feature that will be used mostly by more experienced developers. If they need it, they can figure it out, based on documentation provided.

I'm having pretty hard time trying to explain how I envision reset flag working in .eslintrc. In my mind it's very clear and not confusing at all. So what I'm going to do, is try to create a minimal amount of changes required to make it work that way, and create a WIP request in the next few days. I think that would be much clearer way to explain how I think it should work without trying to draw diagrams with words.

Btw, to be clear, I'm actually not that against having no default rules in ESLint. I just think it's waaaaay too late to make that change now. I think we have to live with what we have at this point.

nzakas commented 9 years ago

I'll try to respond to each point.

  1. This is a silent change. If I started a new project, and I like all of my quotes to be double, but didn't specify that in my .eslintrc file because I get it directly from ESLint base, once this update hits, unless I read release notes (which not everyone does, GA says that each blog entry gets no more then 1000 visitors for the entire life of the site, and npm says that eslint was downloaded 9180 times just today!) I will have no idea that quotes rule is now disabled, and my code is not being checked for it anymore.

A major semver bump is anything but a silent change. People will not get updated without manually specifying latest or the version. So, it's not like there is zero heads up that something is changing. Yes, if you install blindly, things just went work, but that's no different than any other breaking change.

  1. Simple search for ".eslintrc" in github alone returns 5000+ results. That means that 5000+ people will have to go and modify their .eslintrc files once this update hits.

Again, we are removing a bunch of rules and making other breaking changes. These people will need to change these files regardless of this change. We are trying to make 1.0.0 the last time such a thing happens.

  1. Please keep in mind internet phenomenon of vocal minority vs. silent majority. ESLint has been downloaded 185165 time in this month alone, yet I've only seen 5-10 people complaining about defaults. You are basically making majority suffer in order to please very small minority.

We can make this same argument for almost every issue we have. We rarely get more than a handful of reports for any given issue. On the flip side, you are the only one vehemently arguing not to make this change in this thread, so are you the vocal minority here? :)

  1. As far as I'm aware, every linter out there comes with the set of defaults, so people are used to it.

Indeed, we will still have recommended defaults that people can choose to use, just like today. `extends: "eslint:recommended" would be the way that happens.

  1. Having reset in .eslintrc file might be hard to understand, but I think it's a feature that will be used mostly by more experienced developers. If they need it, they can figure it out, based on documentation provided.

Why should this feature be limited to just a subset of users?

I really appreciate you looking out for our users like this. What I'd like to hear from you is why the approaches I've outlined in my previous comments don't address your concerns. I think I have a good idea of your objections, it just seems like what I've described would address them pretty well. If you could explain why my solutions aren't good enough, it would really help me understand better.

In the meantime, I've tweeted and retweeted this thread to give as many people as possible the chance to weigh in.

ilyavolodin commented 9 years ago

The reason why your suggestions didn't address my concerns is mostly because of the shear number of people who will be affected and because it's silent change. In most of the cases where we added breaking changes before, they were to add a new rule that's enabled by default or by retiring existing rule. Both of those actions have instant feedback the first time you are going to run eslint after upgrade. As far as I remember, we never turned a rule off that was on by default before. In the case like that, you will not get any notification at all, the errors in your code would just stop being caught. And that's a really bad thing. People entrust quality of their code to our tool. Yes, 1.0.0 is a major version upgrade, but I'm not sure I can remember going to an OSS project page to read release notes on dependency upgrade. I usually just upgrade and check if nothing breaks. If something broke, then I might go to the site to read release notes. But maybe I'm minority on this issue. I can accept that. Since nobody else seems to be against this change, I guess majority wins.

pascalduez commented 9 years ago

Although I was a bit concerned at first, I'm now reassured and even convinced it's an important and judicious behavior change. Thanks to config inheritance.

without a config, we would still report syntax errors.

That's an important point. It's not like it would do nothing.

extends: "eslint:recommended" is explicit and close to programming paradigms. A newcomer or coworker can have a look at the config and instantly understand where the rules come from. Whereas reset: true is more cryptic.

Personally, when I do a major semver update of a dependency, I seek for the changelog (if any), additionally to run tests. I often had breaking changes, and prefer to read the explanation prior to spend time in debugging. But yes, most probably not everyone do it.

MoOx commented 9 years ago

Here are my 2 cents as a simple user:

I must agree that i dont like having to define a lot of rules.

Extend idea is nice. What about using the opposite of eslint:recommanded and propose something like eslint:reset ?

Most people are happy and love eslint as it is (i am), dont forget that ;)

I really like having a short config mostly for code style and rely on eslint solid default. I think its important for a linter to enforce good practises.

nzakas commented 9 years ago

@ilyavolodin I think where I'm having trouble with your argument is that it's based mostly on the idea that this is to drastic a change for everyone to accept, but we don't have any evidence that it's true. Plus, this change should help us prevent more breaking changes in the future.

@moox with no config, you just get syntax checking. The default rules won't go away, you'll be able to include them in your config via extends and we will provide --init to help get new configs bootstrapped.

lo1tuma commented 9 years ago

I’m one of that persons who configures every single rule. The reset option wouldn’t avoid the problem of overriding every rule configuration because some environments (e.g. the node environment) also enables rules (even stylistic rules).

spadgos commented 9 years ago

I'm in favour of this change. My team has been using eslint since 0.3, and each time we do an upgrade, there's new rules on by default which cause errors and it generally makes the upgrade very messy. We now almost always update the eslintrc files to disable the default-on rules.

I'm in favour of explicit over implicit. Having no rules by default is the clearest and simplest to understand where and why the linter is complaining or not.

I think the argument of looking at the number of current projects which will have to update their config isn't really valid. If you choose to upgrade a package to a new major version number, you should be expecting that some things have changed. Yes, the upgrade path is trickier now, but nothing will automatically upgrade those packages and instantly break things.

ilyavolodin commented 9 years ago

Looks like consensus is for adding reset by default, which I'm fine with. I just voiced my concerns, if consumers are fine going through the upgrade process, I'm also fine with it. I do however have a small suggestion. How about we add a small block of code, that would compare current config object against list of built-in defaults and output a warning if the rule was on by default before and is currently off without explicit setting from the user? We can remove this code in 1.1.0 but at least the upgrade will not be silent (that's my biggest concern, and it hasn't been addressed by any comments above)?

nzakas commented 9 years ago

@lo1tuma this change would mean environments no longer enable rules (which is true of reset today).

@ilyavolodin I'm skeptical that can be done in a useful way. At what point would you check? With the cascade and extends, it's not clear where such a check should go. I'm more of the mind that documentation is the right solution to this problem. We create a 1.0.0 upgrade guide and point out the various things people should check.

ilyavolodin commented 9 years ago

@nzakas I would go for an overkill in this case, and report it for every single file as a warning. We can remove those warning the very next release after 1.0.0, so they wouldn't become annoying. Documentation is good, but we have pretty low traffic on eslint.org in comparison to daily downloads from npm.

nzakas commented 9 years ago

I'm having a hard time with this idea. I definitely am not a fan of the "leave it in for one release" line of thinking, as I think that is far more confusing than anything.

I'm planning on doing a couple of release candidates for 1.0.0 before the final release. I'd rather use that to get some good documentation written than try to be too clever about automated support for the changes. Past visits to the website aren't necessarily a good indicator of what to expect in the future... When people are stuck, they go looking for answers.

doberkofler commented 9 years ago

I personally always configure every rule as I did previously in jslint and jshint and also add comments on what the rule does and why I enable or disabled it. I do understand that using reset as a default is a major change, but believe that in long run only using explicitly defined rules is the way to go.

xjamundx commented 9 years ago

How about we add a small block of code, that would compare current config object against list of built-in defaults and output a warning if the rule was on by default before and is currently off without explicit setting from the user

Or maybe a helpful command-line switch would be eslint --missing-rules which would report any possible rules that have been not configured locally in an .eslintrc file. I know with the large number of new rules I often miss them. Most of the new rules generally are off by default, so that might be nice.

Either of these could be done as 3rd party tools though if we felt they really were that important.

As far as the topic at hand:

I think people will definitely be put off by the change, but in the long term I think we can say with confidence that we're a non-opinionated linting tool and actually mean it. Combined with providing an easy way to extend existing style guides I think this will actually turn out to be a killer feature ;-)

Let me know how I can help....