Enhance linting using airbnb's ESLint rules

coryhouse / react-slingshot

React + Redux starter kit / boilerplate with Babel, hot reloading, testing, linting and a working example app built in

MIT License

9.74k stars 2.94k forks source link

Enhance linting using airbnb's ESLint rules #35

Closed coryhouse closed 8 years ago

coryhouse commented 8 years ago

See https://github.com/airbnb/javascript and https://www.npmjs.com/package/eslint-config-airbnb

amio commented 8 years ago

What about standardjs? http://standardjs.com/

coryhouse commented 8 years ago

Good question. I'm not a fan myself, because they chose to actually require excluding semicolons. At least today, the majority utilize them. I personally agree with Crockford that we should require them, so I chose ESLint's standards, and augmented them with a variety of React rules.

amio commented 8 years ago

I used to stick with semicolons, while I believe specific style is irrelevant compare to consistency, and I'm glad a style could expand the consistency across community rather than a single code base, I love that idea, and I push myself to get use to semicolon-less style, and now I use to it :sweat_smile:

Anyway, that's my personal thought, all these rules are fine to me.

PS: If the semicolon thing is the only thing that bother you, semistandard is an option too, we all love or loved semicolons ;-)

nickytonline commented 8 years ago

@coryhouse, since this is a starter kit for devs, I wonder what value there is to adding airbnb linting rules by default. The existing rules are already pretty decent. This feels more like an option a user would choose when getting setup with the yet to be implemented Yeoman generator (#20).

coryhouse commented 8 years ago

I'm interested in AirBnB's rules for two reasons:

They're a popular standard. The current settings are my own ad-hoc settings
They are broader than the current settings we're using.

The big potential downside I see is switching midway will likely mean a lot of warnings and errors for projects in flight. Are there other reasons to prefer the current setup over AirBnB's?

nickytonline commented 8 years ago

I agree that it's easier to remove it than add it, so let's keep it as a default as you suggested. I'll try and tackle this later this week.

kwelch commented 8 years ago

The other note I want to point out is that once you are on a more standard version like this, using eslint --fix has better effectiveness when attempting to switch. I also agree this would be great option in Yeoman.(#20)

kwelch commented 8 years ago

Since it was noted that the current rules are "decent" I went left them untouched just cleaned up any that were set the same as airbnb to not shadow their rules.

coryhouse commented 8 years ago

I just tried enabling the AirBnB rules on Slingshot. Ouch. Over 600 issues. I didn't realize how many of their rules conflict. And honestly, there are quite a few I personally dislike:

Required trailing comma
Max line length of 100
Requiring single vs double quotes (feels nitpicky)

That said, I'm fixing some linting issues it's found. :)

Still not sold on using this though.

kwelch commented 8 years ago

I concur some of their rules seem ludicrous. If you need help going back through the errors, once you have them solidified, let me know.

coryhouse commented 8 years ago

I've found quite a few other issues with AirBnB's rules:

no-unused-expressions throws on every single use of should with Chai.
react/jsx-indent expects indentation that my editor can't seem to autoindent. Example include indenting content within tbody and the first line after a return (which it expects to be indented two more spaces than I'd expect for some reason). It also expects case statements within select to not be indented. Problem is, most editors will automatically indent case statements. Ouch.

Bottom-line, to use their config I'd want to disable many rules which reduces the value of it.

coryhouse commented 8 years ago

I just enhanced our linting rules by carefully reviewing airbnb's rules. However, I've decided not to use airbnb directly for two reasons:

There are a number of rules they're using that I find undesirable (some mentioned above). I can disable them (about 6), but, there's a bigger issue...
Everything is reported as a build breaking error. I prefer for trivialities to report as warnings. As @kwelch pointed out, errors break flow. I see no easy way to disable this without manually overriding each rule, which make the preset much less desirable.

coryhouse commented 8 years ago

Just for my own notes, here are the airbnb rules I'd disable:

"comma-dangle": 0,
"max-len": 0,
"object-curly-spacing": 0,
"arrow-body-style": 0,
"react/jsx-space-before-closing": 0,
"react/jsx-indent": 0,
"no-unused-expressions": 0,
"react/jsx-closing-bracket-location" : 0