Closed mgcrea closed 8 years ago
Just added a commit refactoring eslint leveraging Airbnb's config which is a pretty good (strict) starting point. Tried to minimize the changes to the code by adding according rules.
One thing that we should definitely change in my opinion is the indenting. The whole JS world uses spaces (both airbnb and feross/standard styleguide do recommend spaces), as an open-source library, this project should try to closely follow commonly used standards.
I did not want to push the change in this commit (should be a single separated commit) without polling you about it as it is often a "sensitive" subject since changing habits is hard. I won't mind if you do not want to switch.
I'm mostly ok with this. I think there are a lot of good changes in there, but a lot of things are just a matter of taste I think.
E.g. I don't think using forEach
instead of for of
is necessarily "better", so I don't really think it makes sense to change that kind of stuff. I don't feel strongly about balanced spacing in params. Not sure if it really makes sense to change it.
I really like the tooling changes, e.g. the Mocha config etc. Also consistently using template strings is a great idea. And we definitely needed test coverage.
Concerning tabs vs spaces: I'm using a tabstop of 8, but I realise that the rest of the world uses either 2 or 4. I think using a tabstop of 8 makes it more painful to write deeply nested functions. I think that's generally speaking not a bad thing.
Any chance we can keep 5702f15 separate and discuss the changes to the linting rules first?
Thanks for the review, indeed some changes are cosmetic, and a matter of state, I don't really mind about any of them, a few side-notes:
forEach
instead of for of
, I'm not encountering for of
in most of the code I'm looking into so I'm not "used to it", forEach
is shorter of both 1 LOC and 1 variable definition so it looks like a little bit more simple/elegant to me but I'll gladly revert it if you want. I'll see if I can cherrypick the eslint commit to another PR.
@mgcrea It looks like this conflicted with the dependency update PR.
Can we change the forEach
s back to for of
?
Will merge as soon as the conflicts are resolved. Great stuff!
@alexanderGugel, I'm on it.
Travis is green, ready for merge! Thanks for the review.
Checklist
Affected core subsystem(s)
Tests
Description of change
fixtures
and testing.eslintrc
)..tmp/test
folder.mocha.opts
to define mocha options