1577835 - Do a formatting pass on comm-central using Prettier (the actual change)

Status: RESOLVED FIXED

(Thunderbird :: General, task, P3)

Description

[Paul Morris [:pmorris]]

Following on doing a Prettier formatting pass on Calendar code in bug 1577606, do the same for the rest of comm-central. Here are some notes copied from the meta bug 1572047:

"Preparation work is at least adding "curly: all" to eslint and do an intermediary step to fix that up (through mach lint --fix), or otherwise we get the ugly online if-elses where braces are missing."

Comment 1

[Paul Morris [:pmorris]]

Attachment: part1-adjust-eslint-settings-0.patch

1 of 23: Adjust eslint settings for reformatting TB code with Prettier. Includes removing the rules that were added when m-c started using Prettier, as darktrojan mentioned on the meta bug:

Some of those rules were removed from mozilla-central when they turned on Prettier. They were added to our config because we had Prettier disabled and wanted to keep the checks.
https://hg.mozilla.org/comm-central/rev/1ba917da139c6e707d6d964c5d1de142cab1e666

These 23 patches should be applied on top of:

• part 1 patch from bug 1573670
• parts 1-7 from bug 1577606
• parts 2-3 from bug 1573670

Comment 2

[Paul Morris [:pmorris]]

Attachment: part2-reformat-chat-code-0.patch

2 of 23: Automated reformatting of chat/ code.

These changes were achieved by:

1. Using eslint to add curly brackets to control
   statements that did not have them (if, else, etc.),
   thanks to the eslint rule: "curly": ["error", "all"]

Done by running |mach eslint chat/ --fix|

2. Reformatting the code using Prettier.

Done by deleting the chat/ line from
the .prettierignore file and running:
|mach eslint chat/ --fix|

This was also the procedure for the other patches that do auto-formatting on other directories besides chat/.

Comment 3

[Paul Morris [:pmorris]]

Attachment: part3-fix-linting-errors-in-chat-0.patch

3 of 23: Manually fix linting errors produced by (left over from) auto-formatting chat/ code.

Comment 4

[Paul Morris [:pmorris]]

Attachment: part4-if-else-comments-chat-0.patch

4 of 23: Fix placement of comments in if/else blocks that were not handled well by auto-formatting (due to insertion of curly brackets).

The insertion of curly blocks worked well overall, but fixing these comments was a chore. Basically if the original code had things like the following, the comment placement was off after the auto-formatting. The comments ended up outside the curly braces.

if (foo) // blah 1 (comments here were handled correctly)
bar();
else // blah 2
baz();

if (foo)
bar() // blah 1
else
baz() // blah 2

Comment 5

[Paul Morris [:pmorris]]

Attachment: part5-remove-chat-eslintrc-file-0.patch

5 of 23: Remove the .eslintrc.js file for chat/ code. These eslint rules for the chat/ code are no longer needed. Removing them did not produce any eslint errors.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part6-reformat-common-ldap-code-0.patch

6 of 23: Auto-format code in the common/ and ldap/ directories.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part7-reformat-editor-code-0.patch

7 of 23: Reformat editor/ code.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part8-fix-linting-errors-in-editor-0.patch

8 of 23: Fix linting errors left over after auto-formatting editor/ code.

Comment 9

[Paul Morris [:pmorris]]

Attachment: part9-if-else-comments-editor-0.patch

9 of 23: Fix comment placement in if/else blocks in editor/ code.

Comment 10

[Paul Morris [:pmorris]]

Attachment: part10-reformat-mail-code-0.patch

10 of 23: Auto-format mail/ code.

Comment 11

[Paul Morris [:pmorris]]

Attachment: part11-fix-curly-lint-errors-in-mail-0.patch

11 of 23: Fix "curly" eslint errors in XUL files in mail/ directory. These errors came from enabling the "curly" rule, rather than the auto-formatting changes.

Comment 12

[Paul Morris [:pmorris]]

Attachment: part12-for-loops-in-mail-0.patch

12 of 23: Fix some atypical for loops in mail/ code that are objectionable to Prettier.

Comment 13

[Paul Morris [:pmorris]]

Attachment: part13-fix-disable-comments-mail-0.patch

13 of 23: Fix placement of "eslint-disable-" comments in mail/ code so they still work.

Comment 14

[Paul Morris [:pmorris]]

Attachment: part14-fix-no-useless-concat-mail-0.patch

14 of 23: Fix "no-useless-concat" eslint errors left over after auto-formatting, in mail/ code.

Comment 15

[Paul Morris [:pmorris]]

Attachment: part15-else-comments-mail-0.patch

15 of 23: Fix placement of comments connected to else blocks in mail/ code.

Comment 16

[Paul Morris [:pmorris]]

Attachment: part16-if-comments-mail-0.patch

16 of 23: Fix placement of comments connected to if blocks in mail/ code.

Comment 17

[Paul Morris [:pmorris]]

Attachment: part17-reformat-mailnews-code-0.patch

17 of 23: Auto-format mailnews/ code.

Comment 18

[Paul Morris [:pmorris]]

Attachment: part18-for-loops-in-mailnews-0.patch

18 of 23: Fix atypical for loops in mailnews/ code that Prettier finds objectionable.

Comment 19

[Paul Morris [:pmorris]]

Attachment: part19-fix-no-useless-concat-mailnews-0.patch

19 of 23: Fix "no-useless-concat" eslint errors left over after auto-formatting in mailnews/ code.

Comment 20

[Paul Morris [:pmorris]]

Attachment: part20-else-comments-mailnews-0.patch

20 of 23: Fix placement of comments connected to else blocks in mailnews/ code.

Comment 21

[Paul Morris [:pmorris]]

Attachment: part21-if-comments-mailnews-0.patch

Comment 22

[Paul Morris [:pmorris]]

The patch in comment 21 is:
21 of 23: Fix placement of comments connected to if blocks in mailnews/ code.

Comment 23

[Paul Morris [:pmorris]]

Attachment: part22-function-comments-mailnews-0.patch

22 of 23: Fix some comments connected with function declarations in mailnews/ code. The auto-formatting mangles cases like this:

/* globals openHelp */// Suite only.
function doHelpButton() {

So put comments on separate lines instead.

/* globals openHelp */
// Suite only.
function doHelpButton() {

Comment 24

[Paul Morris [:pmorris]]

Attachment: part23-reformat-eslintrc-files-0.patch

23 of 23: Auto-format eslintrc.js files with Prettier (using mach eslint --fix). Seems like we'd want to be consistent and format these files with Prettier as well.

Comment 25

[Paul Morris [:pmorris]]

I just tried to do a try server run, but the tree is closed (see bug 1578014).

An earlier try run[0] was successful, except it had an earlier version of the last patch (#23) that did not reformat all of the eslintrc.js files. That problem is fixed in the patches I just uploaded. Aside from patch 23, the only change in the uploaded patches and those in that try server run were rebasing them on the current tip of trunk. The good news is that tests are passing with these changes.

[0] https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=365deb5d9f12b48d6068050b62c86c7f0db39a74

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9089639 [details] [diff] [review]
part2-reformat-chat-code-0.patch

Review of attachment 9089639 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/imContentSink.jsm
@@ +12,5 @@
> +  // default one
> +  // useful if you want to allow or forbid
> +  // an additional thing in a specific
> +  // conversation but take into account all
> +  // the other global settings.

these could use some fixing...

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9089655 [details] [diff] [review]
part16-if-comments-mail-0.patch

Review of attachment 9089655 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/editContactPanel.js
@@ +193,5 @@
>    },
>  
>    deleteContact() {
>      if (this._cardDetails.book.readOnly) {
> +      /* Double check we can delete this. */

I'd make these // style while we're at it.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9089659 [details] [diff] [review]
part20-else-comments-mailnews-0.patch

Review of attachment 9089659 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/am-prefs.js
@@ +44,5 @@
>            newprefstr += prefPartsArray[i] + ".";
>          }
>        }
> +    } else {
> +      /* token is falsy. */

// style please

::: mailnews/base/util/JXON.js
@@ +147,5 @@
>        oParentObj instanceof String ||
>        oParentObj instanceof Number ||
>        oParentObj instanceof Boolean
>      ) {
> +      /* verbosity level is 0 */

here too

@@ +156,5 @@
>  
>      for (var sName in oParentObj) {
>        vValue = oParentObj[sName];
>        if (isFinite(sName) || vValue instanceof Function) {
> +        /* verbosity level is 0 */

here and the others in this patch too

::: mailnews/db/gloda/modules/datastore.js
@@ +1795,2 @@
>        return null;
> +    } else if (

please make it just if. (no else after return)

Comment 29

[Paul Morris [:pmorris]]

Thanks for the review. I've addressed all the comments and rebased on current tip. Uploading new patches next. Try run is underway here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5f2092a05610a9fe5e0226120dfdebd5727ff083

Comment 30

[Jorg K (CEST = GMT+2)]

Can you do some folding to reduce the number of patches?

Comment 31

[Jorg K (CEST = GMT+2)]

Like one patch per directory.

Comment 32

[Paul Morris [:pmorris]]

Attachment: part1-adjust-eslint-settings-1.patch

Comment 33

[Paul Morris [:pmorris]]

Attachment: part2-reformat-chat-code-1.patch

Comment 34

[Paul Morris [:pmorris]]

Attachment: part3-fix-linting-errors-in-chat-1.patch

Comment 35

[Paul Morris [:pmorris]]

Attachment: part4-if-else-comments-chat-1.patch

Comment 36

[Paul Morris [:pmorris]]

Attachment: part5-remove-chat-eslintrc-file-1.patch

Comment 37

[Paul Morris [:pmorris]]

Attachment: part6-reformat-common-ldap-code-1.patch

Comment 38

[Paul Morris [:pmorris]]

Attachment: part7-reformat-editor-code-1.patch

Comment 39

[Paul Morris [:pmorris]]

Attachment: part8-fix-linting-errors-in-editor-1.patch

Comment 40

[Paul Morris [:pmorris]]

Attachment: part9-if-else-comments-editor-1.patch

Comment 41

[Paul Morris [:pmorris]]

Attachment: part10-reformat-mail-code-1.patch

Comment 42

[Paul Morris [:pmorris]]

Attachment: part11-fix-curly-lint-errors-in-mail-1.patch

Comment 43

[Paul Morris [:pmorris]]

Attachment: part12-for-loops-in-mail-1.patch

Comment 44

[Paul Morris [:pmorris]]

Attachment: part13-fix-disable-comments-mail-1.patch

Comment 45

[Paul Morris [:pmorris]]

Attachment: part14-fix-no-useless-concat-mail-1.patch

Comment 46

[Paul Morris [:pmorris]]

Attachment: part15-else-comments-mail-1.patch

Comment 47

[Paul Morris [:pmorris]]

Attachment: part16-if-comments-mail-1.patch

Comment 48

[Paul Morris [:pmorris]]

Attachment: part17-reformat-mailnews-code-1.patch

Comment 49

[Paul Morris [:pmorris]]

Attachment: part18-for-loops-in-mailnews-1.patch

Comment 50

[Paul Morris [:pmorris]]

Attachment: part19-fix-no-useless-concat-mailnews-1.patch

Comment 51

[Paul Morris [:pmorris]]

Attachment: part20-else-comments-mailnews-1.patch

Comment 52

[Paul Morris [:pmorris]]

Attachment: part21-if-comments-mailnews-1.patch

Comment 53

[Paul Morris [:pmorris]]

Attachment: part22-function-comments-mailnews-1.patch

Comment 54

[Paul Morris [:pmorris]]

Attachment: part23-reformat-eslintrc-files-1.patch

Comment 55

[Paul Morris [:pmorris]]

jorgk: just saw your comment 30 & 31. (Wish I'd seen them before uploading all those patches...)

I'd prefer to keep the automatic formatting patches and manual fix patches separate. What about two patches for each directory, one automatic and one manual follow ups?

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d62d63b2f73c
Adjust eslint config for reformatting. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e57378725085
Reformat chat/ code with eslint and Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/8e3167f4cea7
Fix remaining linting errors in chat/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/73f7aa520703
Fix 'if, else, comments', other formatting in chat/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/23725f0afc96
Remove now unneeded chat/.eslintrc.js file. r=mkmelin
https://hg.mozilla.org/comm-central/rev/ab9b63f9d226
Reformat common/ and ldap/ code. r=mkmelin
https://hg.mozilla.org/comm-central/rev/8d3544b63528
Reformat editor/ code with eslint and Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/b1283c57f051
Fix remaining linting errors in editor/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/81f2db384ad9
Fix 'if, else, and comments' formatting in editor/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/883449e17f9d
Reformat mail/ code with eslint and Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/c9b6c0d8e2d2
Fix "curly" eslint errors in xul files in mail/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/90e206d351f7
Rewrite atypical for loops in mail/ for Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/50a23b185cdd
Fix location of 'eslint-disable-' comments in mail/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/f19f1829633f
Fix eslint "no-useless-concat" errors in mail/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e8bbfa11d96e
Fix 'else and comments' formatting in mail/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/3aceadebffb5
Fix 'if and comments' formatting in mail/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5b47088d84e7
Reformat mailnews/ code with eslint and Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/b53bee47da4e
Rewrite atypical for loops in mailnews/ for Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/77773ecb02ca
Fix eslint "no-useless-concat" errors in mailnews/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/90b92315e8a5
Fix 'else and comments' formatting in mailnews/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/d61a07ada51d
Fix 'if and comments' formatting in mailnews/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/dfc10500bb00
Fix 'function and comments' formatting in mailnews/. r=mkmelin
https://hg.mozilla.org/comm-central/rev/8177b85d18db
Reformat the .eslintrc.js files with Prettier. r=mkmelin

Comment 57

[Jorg K (CEST = GMT+2)]

Nice job. However: Two patches had the wrong bug number (check it on your try run), one patch said "But" instead of "Bug". That was the 9th patch which made the entire push fail. So I had to strip and reapply 15 patches which "hg qfinish -a" had already removed :-(

Comment 58

[Paul Morris [:pmorris]]

Oof, oh man, sorry about those slip ups, and thank you for handling them.

Comment 59

[alta88]

Did this all land without ignore-this-changeset or adding to .hg-annotate-ignore-revs and thus destroying useful blame history?

Comment 60

[Jorg K (CEST = GMT+2)]

No, all the reformat patches had ignore-this-changeset, for example here:
https://hg.mozilla.org/comm-central/rev/5b47088d84e7

Somehow "ignore-this-changeset" doesn't work out of the box, some measures need to be taken for those changesets to be really ignored.

Comment 61

[alta88]

Well, when looking here in searchfox, blame shows this commit:
https://searchfox.org/comm-central/source/mail/base/content/browserRequest.js#19
Should it?

Comment 62

[Jorg K (CEST = GMT+2)]

Yes, last line of comment #60. I don't know how to ignore them, someone needs to talk to Searchfox.

Comment 63

[alta88]

Perhaps talking to (copying) firefox would be better, and using the second technique in last line of comment 59?
There is a post from Jan on m.dev.platform, subject: Skipping changesets when viewing blame

Comment 64

[Magnus Melin [:mkmelin]]

I think we need a .hg-annotate-ignore-revs (and .git-blame-ignore-revs) file

Comment 65

[Paul Morris [:pmorris]]

Adding .hg-annotate-ignore-revs and .git-blame-ignore-revs files in bug 1579183 and bug 1579191.