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

Enable auto-formatting of the entire code-base using Prettier (issue 11444) #11446

Merged
merged 3 commits into from
Dec 26, 2019

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Dec 25, 2019

Note that Prettier, purposely, has only limited configuration options. The configuration file is based on the one in mozilla central with just a few additions (to avoid future breakage if the defaults ever changes).

Prettier is being used for a couple of reasons:

  • To be consistent with mozilla-central, where Prettier is already in use across the tree.

  • To ensure a consistent coding style everywhere, which is automatically enforced during linting (since Prettier is used as an ESLint plugin). This thus ends "all" formatting disussions once and for all, removing the need for review comments on most stylistic matters.

Many ESLint options are now redundant, and I've tried my best to remove all the now unnecessary options (but I may have missed some).
Note also that since Prettier considers the printWidth option as a guide, rather than a hard rule, this patch resorts to a small hack in the ESLint config to ensure that comments won't become too long.

Please note: This patch is generated automatically, by appending the --fix argument to the ESLint call used in the gulp lint task. It will thus require some additional clean-up, which will be done in a separate commit.

(On a more personal note, I'll readily admit that some of the changes Prettier makes are extremely ugly. However, in the name of consistency we'll probably have to live with that.)

Fixes #11444.

… data structures

There's a fair number of (primarily) `Array`s/`TypedArray`s whose formatting we don't want disturb, since in many cases that would lead to the code becoming much more difficult to read and/or break existing inline comments.

*Please note:* It may be a good idea to look through these cases individually, and possibly re-write some of the them (especially the `String` ones) to reduce the need for all of these ignore commands.
…11444)

Note that Prettier, purposely, has only limited [configuration options](https://prettier.io/docs/en/options.html). The configuration file is based on [the one in `mozilla central`](https://searchfox.org/mozilla-central/source/.prettierrc) with just a few additions (to avoid future breakage if the defaults ever changes).

Prettier is being used for a couple of reasons:

 - To be consistent with `mozilla-central`, where Prettier is already in use across the tree.

 - To ensure a *consistent* coding style everywhere, which is automatically enforced during linting (since Prettier is used as an ESLint plugin). This thus ends "all" formatting disussions once and for all, removing the need for review comments on most stylistic matters.

Many ESLint options are now redundant, and I've tried my best to remove all the now unnecessary options (but I may have missed some).
Note also that since Prettier considers the `printWidth` option as a guide, rather than a hard rule, this patch resorts to a small hack in the ESLint config to ensure that *comments* won't become too long.

*Please note:* This patch is generated automatically, by appending the `--fix` argument to the ESLint call used in the `gulp lint` task. It will thus require some additional clean-up, which will be done in a *separate* commit.

(On a more personal note, I'll readily admit that some of the changes Prettier makes are *extremely* ugly. However, in the name of consistency we'll probably have to live with that.)
…t `--fix` couldn't handle

This patch makes the follow changes:
 - Remove no longer necessary inline `// eslint-disable-...` comments.
 - Fix `// eslint-disable-...` comments that Prettier moved down, thus causing new linting errors.
 - Concatenate strings which now fit on just one line.
 - Fix comments that are now too long.
 - Finally, and most importantly, adjust comments that Prettier moved down, since the new positions often is confusing or outright wrong.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/e96b4de3a2a9b66/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/e96b4de3a2a9b66/output.txt

Total script time: 1.74 mins

Published

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/3dacbc2adfd57a9/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/5759f99a92b50c3/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/3dacbc2adfd57a9/output.txt

Total script time: 18.89 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/5759f99a92b50c3/output.txt

Total script time: 25.80 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@Snuffleupagus Snuffleupagus marked this pull request as ready for review December 26, 2019 12:27
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Dec 26, 2019

While the first and last commits should be possible to review manually, the main commit is unfortunately not feasible to review (and enabling Prettier everywhere at once seemed like a good idea).
Hence it's probably going to be necessary to trust that ESLint --fix did the "right" thing here, and also that I ran the command correctly :-)

@Snuffleupagus
Copy link
Collaborator Author

/botio lint

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_lint from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/1fc5ea99334c055/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_lint from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/7f45de869546954/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/1fc5ea99334c055/output.txt

Total script time: 1.17 mins

  • Lint: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/7f45de869546954/output.txt

Total script time: 2.58 mins

  • Lint: Passed

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Dec 26, 2019

Once Prettier auto-formatting has landed: I think that we should avoid doing things like e.g. the recent prefer-const conversion on a file-by-file basis, and rather do those things globally and/or on a directory basis instead (since I'm not crazy about the code churn the previous method results in).

@timvandermeij timvandermeij merged commit 81af207 into mozilla:master Dec 26, 2019
@timvandermeij
Copy link
Contributor

Thank you for improving this! I think it's a huge step forward in terms of consistency with mozilla-central and in the code base itself. It will also help during development work and reviews.

I checked everything except for the main commit, but I did look over it to see if all changes look correct. I'm indeed going to trust that ESLint did the work correctly, and the passing tests and linting prove this.

I have reviewed this pull request first because I figured rebasing this would be horrible, so now we can rebase the other patches easily.

@Snuffleupagus Snuffleupagus deleted the prettier branch December 26, 2019 22:54
@Snuffleupagus
Copy link
Collaborator Author

Thanks a lot for taking the time to review this; and also for prioritizing getting this landed quickly to avoid painful rebasing :-)

@wojtekmaj
Copy link
Contributor

Holy crap, that's how to enter 2020 in style! 😎 Congratulations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test-drive Prettier
4 participants