Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Use eslint + prettier for linting#19302

Merged
rafeca merged 8 commits intomasterfrom
use-eslint-for-linting
May 13, 2019
Merged

Use eslint + prettier for linting#19302
rafeca merged 8 commits intomasterfrom
use-eslint-for-linting

Conversation

@rafeca
Copy link
Contributor

@rafeca rafeca 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 jasonrudolph and nathansobo May 10, 2019 12:10
Copy link
Contributor

@nathansobo nathansobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

rafeca 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 Compare May 13, 2019 15:48
@rafeca rafeca merged commit fe95e05 into master May 13, 2019
@rafeca rafeca deleted the use-eslint-for-linting branch May 13, 2019 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants