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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable prettier #19391

Merged
merged 2 commits into from May 31, 2019

Conversation

Projects
None yet
1 participant
@rafeca
Copy link
Contributor

commented May 23, 2019

Summary

This PR is a follow-up of #19302 (next step 1. from the summary of that PR) to enable the prettier eslint config and reformat all JS files to follow it.

This means that once this is merged, the linter will complain whenever a file does not have the correct prettier formatting (this includes whitespace, line lengths, etc).

Good thing is that fixing the formatting is as easy as running script/lint --fix from the command line, or even better you can install the prettier-atom package and turn on the "format on save" setting to automatically format the code (this changed my life when I started using it).

In order to create this PR, in the first commit I've just modified the .eslintrc.json file to enable the prettier rule and tweaked the prettier config to use semicolons, which is the standard configuration of prettier (as suggested by @nathansobo).

On the second commit I've reformatted all the files by running script/lint --fix.

Motivation

This will provide a few benefits:

  • Better developer experience: no more manual formatting of files (e.g indent lines correctly or care about line lengths).
  • More style consistency: All code will use the same formatting (currently things like line length or how to split long lines are not standarized across the codebase).

Drawbacks

This commit touches a toon of files and lines, and this adds a few drawbacks:

  • Blame history will get polluted by this commit (good thing is that the changes are "simple" so skipping through this commit while blaming is trivial).
  • Code conflicts are going to appear when merging PRs that were in progress while this PR get merged.
  • Code conflicts can appear if we try to cherry-pick commits from master to any release branch after this PR gets merged.

How I'm planning to merge this

Taking into account the drawbacks, this is my current strategy to merge this if nobody sees issues with this PR:

  1. Send this PR today (May 23rd) in order to gather feedback about it.
  2. Once there's agreement (or at most at EOD May 24th), notify on #atom-maintainers about this PR, so if anybody has a big WIP branch they have some time to merge it.
  3. One week later (May 31st), I'll rebase this branch on master, re-generate the reformatting commit and merge it.
  4. ...
  5. Profit!

The dates are carefully chosen so this will land 1 week before the branch cut for the next release. I think this is the sweet spot by being early enough to have enough time to spot any unexpected issue (although the changes should only affect formatting) and being late enough so the chances of us having to cherry-pick a commit from master into a release branch are very low.

Questions

I have a big WIP branch which is not going to be ready by May 31st, what should I do?

Next time you should try to work more incrementally 馃榿. Nah just kidding, if your branch focuses on a specific file/files or adds new files, you can already enable prettier on your branch files, so they get formatted correctly before this PR gets merged and you won't get conflicts. To do so, you can just add a new override on the eslintrc config:

{
  "files": ["globs_that_match_your_pr_files"],
  "rules": {
    "prettier/prettier": ["error"]
  }
}

Once you do that, you can run script/lint --fix and your PR files will get auto-formatted.

What happens with the electron upgrade?

Luckily, the current upgrade branch does not touch many JS files, so there are not going to be many conflicts.

Having said that, if on May 31st we see that there's still possibilities that we need to revert the electron branch, I'll consider postponing the merge of this PR.


/cc @atom/maintainers

@rafeca rafeca force-pushed the use-prettier branch 3 times, most recently from ac14365 to 5eb892b May 23, 2019

@rafeca rafeca requested review from as-cii, nathansobo and jasonrudolph May 25, 2019

@rafeca rafeca force-pushed the use-prettier branch from 5eb892b to e662b13 May 29, 2019

@rafeca rafeca force-pushed the use-prettier branch from e662b13 to 7f3f040 May 31, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

I'm going to merge this PR in a few minutes. Once this is merge I'll fix the conflicts of all the PRs on atom/atom that fulfil the following conditions:

  • Have been updated during 2019.
  • They grant permissions to atom contributors to get updated.
  • Don't have conflicts already.

On the rest of PRs that have been updated during 2019 I'll leave a comment with instructions about how to fix the conflicts.

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