Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use eslint + prettier for linting #19302

Merged
merged 8 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@rafeca
Copy link
Contributor

commented May 10, 2019

As a followup of @nathansobo's change, this PR changes the linting infra of Atom to use eslint directly, instead of standard.

This allows a full integration with prettier, better support for new syntax features and many more rules that were not enforced before.

In order to keep this PR small and reviewable, I've disabled the prettier/prettier rule for now in the eslint config, plus some other rules that were causing many errors.

So there are a couple of next steps to do after this PR:

1. Enforce prettier rules

I've prepared everything so this can be done super easily after this PR by setting the prettier/prettier eslint rule to error and then run script/lint --fix.

This will run prettier through the whole codebase (more specifically the files covered by eslint) and reformat them so they follow the prettier conventions. This will basically change whitespace since the prettier configuration has similar style rules as standardjs.

Once this is done, the linter will fail whenever a file does not have the correct formatting dictated by prettier. This can be fixed by either running prettier from an editor or by running script/lint --fix.

Since enabling prettier will change a looot of files (I just tried it and it modified 9k files), if we're concerned about merge conflicts or other potential problems we can also enable it progressively on some folders first by adding an eslint override.

2. Enable some disabled rules

This is not urgent, but I have disabled some newer eslint rules for now since they cause many errors that we can fix later.

Since these rules were not enforced before, that's not a big issue, but most of them seem to be fair rules that make code less prone to errors.

When to do the prettier reformatting?

Reformatting all files can be a bit disruptive in two situations:

  1. Existing PRs, which can get conflicts and will need to get rebased.
  2. Cherry-picking changes to release branches can lead to conflicts.

I don't see 1. as a big problem, but 2. can be quite annoying when we can to cherry-pick things to the current beta version. In order to minimize this one, we should merge the prettier reformatting PR just before rolling the railcars for a new release (so both master and the latest release branch will have the new code).

@rafeca rafeca requested review from nathansobo and jasonrudolph May 10, 2019

Show resolved Hide resolved .eslintrc.json
Show resolved Hide resolved .eslintignore
@nathansobo
Copy link
Contributor

left a comment

Looks great. In the follow-up PRs that you mention... if we're going to be doing a mass reformat, I wonder if it makes sense to just switch our linting rules to prettier's defaults at that time. The downside would be a bit more churn for the files that are already formatted with prettier+standard rules, but I imagine that PR would touch quite a few files anyway.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Looks great. In the follow-up PRs that you mention... if we're going to be doing a mass reformat, I wonder if it makes sense to just switch our linting rules to prettier's defaults at that time. The downside would be a bit more churn for the files that are already formatted with prettier+standard rules, but I imagine that PR would touch quite a few files anyway.

Sounds good to me! The two changes of the default prettier config would be:

  • Add semicolons (yay!)
  • Double quotes for strings (I'll have to get used to this one, but since prettier takes care of the formatting I won't care that much).

@rafeca rafeca force-pushed the use-eslint-for-linting branch from d41945a to 4048b49 May 13, 2019

@rafeca rafeca merged commit fe95e05 into master May 13, 2019

2 checks passed

Atom Pull Requests #20190513.6 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the use-eslint-for-linting branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.