Skip to content

tests(eslint): update eslint (and goog config) to latest#3396

Merged
patrickhulce merged 2 commits intomasterfrom
eslint
Oct 11, 2017
Merged

tests(eslint): update eslint (and goog config) to latest#3396
patrickhulce merged 2 commits intomasterfrom
eslint

Conversation

@brendankenny
Copy link
Contributor

@brendankenny brendankenny commented Sep 23, 2017

this was going to be simple, but eslint 4.x got stricter with indentation, so some decisions had to be made. The biggest one was standardizing what indentation happens before a dot access (memberExpression) split to the next line. Given the amount of promise chaining in the project (which has typically only gone one level deep), the far less disruptive change was to standardize on a single level of indentation for dot accesses while leaving everything else at two levels.

The new eslint is also really strict about ternary expressions for some reason (while completely ignoring indentation levels for the continuations of regular BinaryExpressions, for instance), with no way to customize, so I just made it ignore indentation at the top level for those. Try not to abuse that :)

This change also adds eqeqeq to our rules (with only two == corrections to be made, yay), and runs eslint on the .eslintrc.js files themselves cause why not.

It might be slightly easier to look commit-by-commit, as the first is a bunch of indentation changes (and removal of extra blank lines and useless regex escapes, also now better enforced by eslint) but all the rest of the commits are pretty limited in scope.

@brendankenny
Copy link
Contributor Author

oh, and https://github.com/GoogleChrome/lighthouse/pull/3396/files?w=1 makes it super easy to look over the much smaller amount of non-whitespace changes

@wardpeet
Copy link
Collaborator

@brendankenny maybe next step is moving to prettier? Might fix the issue with indentation and ternary operators

@paulirish
Copy link
Member

I took this branch and then applied prettier to it, to get a sense of the diff. Looks like it keeps much of the changes this PR made and then massages a few more parts. Seems p good, TBH.

eslint...paulirish:prettier

@patrickhulce
Copy link
Collaborator

patrickhulce commented Sep 25, 2017

I ❤️ prettier && eslint --fix

@brendankenny
Copy link
Contributor Author

maybe next step is moving to prettier? Might fix the issue with indentation and ternary operators

maybe :) Prettier doesn't fix the indentation either, it just picks a different one.

We could maybe write our own indentation rule just for binary and conditional expressions so eslint --fix would work (unfortunately it looks like eslint won't be fixing this for "backwards compatibility", at least any time soon)

@brendankenny
Copy link
Contributor Author

updated!

@brendankenny brendankenny changed the title test(eslint): update eslint (and goog config) to latest tests(eslint): update eslint (and goog config) to latest Oct 10, 2017
@patrickhulce
Copy link
Collaborator

how much churn is there for sticking with 2 space indentation on all continuations? Is there a strong objection to adopting that everywhere?

doing

return myMethod(
    superverylongarg1,
    superverylongarg2,
    superverylongarg3
)

with 4 spaces feels wrong (if only just because I trust prettier on being more consistent than my eyes :) )

@brendankenny brendankenny force-pushed the eslint branch 2 times, most recently from 17f498a to 1582c38 Compare October 11, 2017 00:41
@brendankenny
Copy link
Contributor Author

ok, kicking the "standardizing indentation" question down the road, much simpler changes to use latest eslint :)

Now mostly removing extra padding spaces/lines, unnecessary regex escapes, and making the eslintrc.js files follow their own style

@patrickhulce
Copy link
Collaborator

thanks for the slog here @brendankenny! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants