Javascript: consider removing stylistic rules that can't be automatically fixed from eslint-config-airbnb

[jedwards1211]

I'm dismayed at how often I have to fight with some of these rules in open-source projects that use them; to one who's used to automatic code formatters cleaning up things like indentation and spacing, having ESLint complain about nitpicky things with your code instead of just fixing them feels demeaning and inhumane.

I also think this is a concerning development in programmer culture given how ESLint now has much vaster nitpicking capability than any kind of automated analysis tools I've ever been aware of. It's like OCD on a mass, collective scale.

If a rule helps prevent bugs, okay, I get it, and I'm happy to comply with it. But manually fixing little stylistic issues that have nothing to do with preventing bugs, because ESLint doesn't yet support automatically fixing them? Come on, relax, it will be okay, get outside and remember there's more to life than slaving away like a robot whose only job is to produce absolutely perfect selfsame code.

As a popular, well-known company, you have the power to influence programmer culture and quality of life for better or worse with this ruleset and other tools you release; please consider their impact.

[hshoff]

Hi @jedwards1211, thanks for checking out the guide. We always appreciate feedback from the community. I totally get where your frustration is coming from--the joy comes from writing the code and making it work, not writing code to please the computer. Yet here we are letting the computer give us feedback on our code. Why are doing this? What's in it for me?

There's a lot in this issue, so let me back up a bit to some first principles we believe in before we get into your proposal.

1. Linter-enforced style guides benefit teams & projects over time
   
   
   
   • The bigger your team gets and the more projects a codebase has, context switching becomes increasingly expensive. See https://github.com/airbnb/javascript/issues/102 for a more in-depth example on the problem a team faces while maintaining codebase quality over time.
   
   
2. Time-saving discussions for everyone
   
   
   
   • When any project adopts a lint step, it becomes much easier for maintainers to accept pull requests from contributors. Maintainers no longer have to comment or close a PR saying "please use 2 spaces" or "please add semicolons" which we've seen spiral out of control into issues about whether semicolons are moral or just in today's world.
   
   
   • The beauty of it is the contributor doesn't have to wait for the maintainer to respond with the necessary fixes--they get immediate feedback. The project moves forward faster and code quality stays consistent.
   
   
   • The discussion is focused solely on what the code does, the implementation details, next steps, etc.
   
   
   • As GitHub has made it easier for programmers to connect and collaborate around the world, this becomes increasingly important because projects now have 24/7 dev time. Why slow everything down waiting on humans to parse code?
   
   
3. Forks encouraged, pull requests welcome
   
   
   
   • Let us know! Open an issue for specific rules that you feel are too nitpicky and we'll do our best to explain our reasoning or detail a way to disable it for your project.
   
   
   • The beauty of ESLint’s plugin system is that you can extend an eslint plugin config and turn off rules you disagree/don't care about, publish it on npm, and take it with you everywhere you go.
   
   
4. Opinions all the way down
   
   
   
   • Many folks, such as yourself, can adapt to any code style and don't have a preference about nitpicky things. But there are others that can't stand it when brackets don't align or when their commit diffs look huge when they only added a new variable.
   
   
   • So how do we reconcile these two types of programmers? Let's agree on how we (team or project) write code, bring in a referee (the linter), and keep the conversation focused on feature, fix, or changeset.
   
   
   • There are a lot of strong opinions in this guide. We try to include our reasoning to help folks see why we made certain decisions. This guide has evolved over time and will continue to be updated as we learn new things. We try to publish new versions responsibly with SemVer and changelogs.
   
   
5. Transition old codebases
   
   
   
   • Codemods are your friend. Check out this terrific post by @lencioni that details how to refactor an older base in an efficient way.
   
   

---

Now to the issue at hand! Your proposal to remove stylistic rules that can't be automatically fixed from eslint-config-airbnb.

With a linter we get the computer to do the boring work of pointing out stylistic issues. But we humans still have to go in and make the fixes. Feels like a half-measure. I agree. I also think eslint --fix and Codemods are getting better all the time and will help everyone make these changes efficiently and easily in the future. For example, ESLint 2.9.0 included some autofix improvements that lay the foundation for many more rules to be autofixed.

The real question is what do we do until the day we have the magic autofix?

Option 1: remove all the other rules and let the code run wild.

Good:

• You run fix and be done with it.

Bad:

• The code review discussions get messy because style opinions love to surface in a code review
• Old parts of the codebase stay old since it's a lot of work to update them.
• Team members slow down as they hop between projects and styles
• PR review slows down

Option 2: keep the rules that can't be fixed as a bridge until the time that they can be autofixed

Good:

• See 1-5 above

Bad:

• It can feel like an uphill battle

For now, we see Option 2's benefits outweighing the downsides in this current bridge state we find ourselves in, but tools are getting better all the time.

So there's a light at the end of the tunnel! We'll get there.

Hope this helps explain the direction and motivation for this project. As always, we appreciate feedback.

[lencioni]

I think there is another benefit to enforcing a consistent style that @hshoff didn't mention. If code is stylistically hyper similar, your pattern recognition facilities work much better, which will allow you to more quickly spot bugs or know how to modify the code to make it do what you want. This is helpful for the author of the code, the reviewers of the changes, and future explorers who wish to understand how it works. Being able to quickly understand the code is very useful in this type of work, and having it all look exactly the same clears away a pile of tiny obstacles on that path.

TL;DR consistent code helps prevent bugs and increases productivity when writing and reviewing code (beyond preventing endless style discussions).

Put another way, some rules prevent bugs and the others prevent future bugs.

[jedwards1211]

Hi @hshoff and @lencioni, thanks for taking the time to write thoughtful responses. You bring up a lot of good points, and have basically convinced me. And it's true that with time this should get better as more things are autofixable by eslint.

lencioni's comment about pattern recognition is interesting, but I think linting/autoformatting is a double-edged sword in this manner; it really depends on the rules you choose. Here are two examples of how it can interfere with pattern recognition (which I'm glad are not part of the airbnb style guide, but have bugged me in other people's eslint settings):

sort-vars, proposed sort-keys, jsx-sort-props, etc.

While having things alphabetized can make them easier to spot by eye, it can potentially cause weird, non-semantic ordering. For instance:

const timeRange = {
  end: ...
  start: ...
}

Using "begin" instead of "start" would sort before end, but unrelated things might wind up in between:

const myPlot = <Plot begin={...} majorSpacing={...} minorSpacing={...} series={...} end={...} />

If there are any issues with the time range being wrong, it may be easier for programmers to figure out what's going on if begin and end are right next to each other, as programmers are likely to order them without a linter.

Note that airbnb's advice to group shorthand props before others could cause similar ordering issues.

no-multi-spaces

Aligning similar things in separate lines can make code a lot more clear. For instance:

  var karma       = options.karma;
  var watch       = options.watch        || false;
  var target      = options.target;
  var metatarget  = options.metatarget;

  var mode        = options.mode         || process.env.NODE_ENV || 'development';
  var test        = options.test;

  var host        = options.host         || '0.0.0.0';
  var entry       = options.entry        || defaultEntry;
  var meteorPort  = options.meteorPort   || 3000;
  var webpackPort = options.webpackPort  || 9000;
  var meteorDir   = options.meteorDir;
  var webpackDir  = options.webpackDir   || __dirname;

To me, with no-multi-spaces enforced, the delineation of default values becomes less clear, and it's significantly harder to see in one quick glance whether all lines are reading something from options:

  var karma = options.karma;
  var watch = options.watch || false;
  var target = options.target;
  var metatarget = options.metatarget;

  var mode = options.mode || process.env.NODE_ENV || 'development';
  var test = options.test;

  var host = options.host || '0.0.0.0';
  var entry = options.entry || defaultEntry;
  var meteorPort = options.meteorPort || 3000;
  var webpackPort = options.webpackPort || 9000;
  var meteorDir = options.meteorDir;
  var webpackDir = options.webpackDir || __dirname;

In general, alignment that reveals that multiple statements have the same structure helps programmers comprehend that structure more quickly. The human brain is good at quickly spotting what parts are repetitious down the page and focusing on the differences.

[ljharb]

The problem with alignment is that you create lots of diff churn when you add a variable that's slightly longer than the others, which makes code reviews harder. Syntax highlighting should be sufficient to indicate default values, assignment, etc - the human brain is just as good at spotting color differences as it is spacing differences.

[lencioni]

@jedwards1211 fun discussion! Regarding sorting, while I agree that semantic ordering can be nice, the "correct" order is also often ambiguous. The beauty of sorting things is that it cuts down on the number of decisions that developers need to make, which reduces decision fatigue, which frees them up to make more important decisions. There are some other side-benefits, such as making it easier to find things in long lists, but I really see those as secondary to fundamentally simplifying the problem space. Simple rules are more likely to be followed.

[jedwards1211]

@ljharb about diff churn on whitespace -- I would argue that's a problem with the tools. Obviously the whitespace changes have to be stored in commits, but as far as code reviews, I've seen diff tools where you can check an "ignore whitespace" option; why GitHub doesn't seem to have that option in its diff views puzzles me. Obviously the initial indentation could be an issue, but ignoring non-initial whitespace would be a reasonable option too. And I'm sorry, I'm not convinced that syntax highlighting alone is as clear as alignment; the above examples are highlighted but I still find the aligned one easier on the eyes.

[lencioni]

You can add ?w=1 to the URL on GitHub to suppress whitespace. IIRC you can't comment while in this view, but it sometimes comes in very handy.

https://github.com/blog/967-github-secrets

I also agree that syntax highlighting alone isn't as clear as alignment, but I think it is pretty good.

[jedwards1211]

Ah, cool! Yes, for instance, if one of the options references in my example above, say in the middle of the bottom section, were replaced with something else, it would be much more obvious in the aligned version than the non-aligned version.

[jedwards1211]

@ljharb so in other words, alignment isn't always done merely to indicate assignment, default values, etc.; it can indicate any number of patterns and help you spot what's the same/different between instances of the pattern, even within identifiers, which are never highlighted with multiple colors.

[jedwards1211]

On the downside, it does take more typing and decision fatigue to maintain. I think alignment could be automated fairly well by a formatter though.

[lencioni]

I can't speak for @ljharb but I think the diff churn concern is much less about reviewing and much more about making git blame less useful.

[jedwards1211]

@lencioni That's a good point. But for the sake of playing devil's advocate, perhaps there should be an ignore whitespace option in git blame?

[lencioni]

Yep. Looks like there is! https://coderwall.com/p/x8xbnq/git-don-t-blame-people-for-changing-whitespaces-or-moving-code

Edit: I asked the git mailing list if we can get config options to turn these options on by default: http://article.gmane.org/gmane.comp.version-control.git/293903

[jedwards1211]

Cool, I should have just thought to check!

[jedwards1211]

Unfortunately other tools like SourceTree don't have an ignore whitespace option when blaming. Which is the kind of reason I tend to dislike 3rd-party git frontends.

https://answers.atlassian.com/questions/273902/is-it-possible-to-call-blame-while-ignoring-white-space-blame-w

[ljharb]

Thanks for the discussion!

Many rules are useful but can't be autofixed, and the benefits of those rules far outweigh any potential fatigue or culture impact. We won't be using "has an autofixer" as a criteria for any linter rule - but we encourage using them, and writing PRs to add them to rules that lack them!

[iyobo]

@hshoff
Option 1: Bads:
The code review discussions get messy because style opinions love to surface in a code review

How is this an issue when you make it extremely clear in your organization that "Styling back and forth is extremely discouraged/prohibited in code reviews as long as it passes the auto-linter"?

If anyone has new code styling suggestions, the code review will not be the place for that discussion. The onus would be on that person to FIRST ensure their suggestion is auto-fixable, then bring it up on our next dev meeting so we can evaluate whether it makes sense to [retro-actively] apply it. It certainly shouldn't trump actual productivity.

I agree with the OP in that I have noticed this issue become a worrying trend in dev culture. There's just too much pandering to OCD thinking, sometimes to the detriment of other people and budget. That's on the team level.

Personally, I don't care what code looks like as long as:

• it works and doesn't cause bugs
• It's performant
• It passes a basic auto-formatter run. e.g Webstorm default format or whatever, as long as it is automatic. It shouldn't even be something to waste brain cells (or budget) on.

If an engineer cannot understand code they are looking at simply because it isn't "styled to their EXACT preference", then that person really should consider another profession.

Old parts of the codebase stay old since it's a lot of work to update them.
eslint fix can autofix an entire directory tree deeply.

Team members slow down as they hop between projects and styles
PR review slows down
No more than the constant-slow-down from a dev always manually ensuring their next PR is extremely following nits. Consider it an investment.

[ljharb]

@iyobo plenty of the rules that aren’t autofixable prevent bugs and improve performance.

[jedwards1211]

@ljharb I think most of us would be fine with rules that prevent bugs or improve performance that are inherently un-autofixable. I use no-unused-vars and no-undef all the time.

The rules I hate the most are the ones that don't do anything other than enforce a consistent style and aren't autofixable.

I've been thinking about this whole argument about decision fatigue. Sometimes you're trading one source of fatigue for another. Prop sorting is a great example: before you're making little decisions about what order makes semantic sense to you, now you're expending mental energy to alphabetize all the time. (That is, unless you're using some tool outside of eslint to automatically sort props?) No mental energy has been saved.

And I get the desire to avoid arguing about little nits during code reviews, but think about it on a case-by-case basis -- has anyone really griped about prop order during a code review? Would you really continue working with that person if they don't agree to let it go?

So I think in the case of some rules like prop sorting there's really no net benefit.

[iyobo]

@ljharb what @jedwards1211 said.

@jedwards1211 "has anyone really griped about prop order during a code review? "

Oh you'd be surprised :D ...

[ljharb]

Yes, people gripe about prop order in code reviews all the time at Airbnb. The reason we don’t specify prop ordering in this style guide is because we have no internal consensus on it: thus, our rule is “nobody is allowed to gripe about prop order, and it’s always up to the PR author, and please avoid unnecessary diffs”.

[jedwards1211]

Ah, well that's good at least. I had forgotten that sort-keys was merely proposed here, not actually in place.