1577606 - Use Prettier for formatting JavaScript in Calendar

Status: RESOLVED FIXED

(Calendar :: General, task, P3)

Description

[Paul Morris [:pmorris]]

An incremental step along the path of bug 1572047. Splitting out the calendar part as a separate effort. There's some calendar-specific discussion already on bug 1572047. The most relevant bits:

• Drop or turn off some eslint rules (generator-star-spacing, quote-props, object-curly-newline) that conflict with Prettier. Just use the Prettier defaults for these like m-c does.
• Use 2 space indent for calendar (per bug 1530221).
• Use a .prettierrc file for calendar-specific Prettier settings (e.g. line width).

Comment 1

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/try-comm-central/rev/996e7b3fa824d6b30d195dcedbc0e1bf4b679371#l1.12
isn't right.
That should be: for (let i = exdates.value.length - 1; i >- 0; i -= 1) { - or i--.

for (let i = 10; i--;) { console.log(${i}\n); } prints 9 to 0.

Comment 2

[Jorg K (CEST = GMT+2)]

I guess, for (let i = 10; i--;) check the look condition before it decrements, so it checks 10 down to 1 and runs with 9 to 0. --i would give a different result. Tricky, huh?

Comment 3

[Paul Morris [:pmorris]]

Thanks Jorg for catching that. I did i > 0 when I should have done i >= 0. Fixed in patches I'm about to upload. I'll be more careful with these atypical for loops. That tricky thing where --i is not the same as i-- is why D. Crockford recommends always just using i -= 1. Avoid the footguns. Maybe that's what threw me.

Comment 4

[Paul Morris [:pmorris]]

Attachment: part1-turn-prettier-on-0.patch

0 of 6: First apply this patch:
https://bug1573670.bmoattachments.org/attachment.cgi?id=9088722
from bug 1573670.

1 of 6: Then apply the patch I'm attaching here which turns on Prettier but keeps it from doing anything by ignoring all directories, etc.

Comment 5

[Paul Morris [:pmorris]]

Attachment: part2-adjust-settings-0.patch

2 of 6. Do settings before reformatting.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part3-reformat-code-0.patch

3 of 6: The big automatic reformatting pass. Details from the commit message:

These changes were achieved by:

(1) Using eslint to re-indent calendar code with 2 space
indent instead of 4.

Done by temporarily adding the following rule to the
calendar/.eslintrc.js file:

"indent-legacy": [2, 2, { SwitchCase: 1, }],

Then temporarily turning off Prettier by adding to that
file the rule:

"prettier/prettier": "off",

Then running |mach eslint calendar/ --fix|

(2) Reformatting the calendar code using Prettier.

Done by removing those two temporary additions from the
calendar/.eslintrc.js file and running
|mach eslint calendar/ --fix|.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part4-fix-eslint-disable-comments-0.patch

4 of 6: This and the following patches fix some linting errors left after auto-formatting. This one moves existing "eslint-disable-" comments so they still work.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part5-fix-for-loop-0.patch

5 of 6: Fix an atypical for loop that Prettier didn't like.

Comment 9

[Paul Morris [:pmorris]]

Attachment: part6-fix-useless-concats-0.patch

6 of 6: Fix "no-useless-concat" eslint errors. Where the strings were separated on purpose for readability I usually kept them separate and added 'eslint-disable-' comments.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089256 [details] [diff] [review]
part1-turn-prettier-on-0.patch

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

I don't think you should list directories where there are no files to format, which from a quick look, are build, db, mozharness, other-licenses, rdf, taskcluster, testing, and third_party. That does mean a little bit more work for the linting task, but also means any file put in one of those folders at a later date must be formatted correctly.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089257 [details] [diff] [review]
part2-adjust-settings-0.patch

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

::: calendar/.eslintrc.js
@@ +28,5 @@
>          // We should get better at complexity, but at the moment it is what it is
>          "complexity": [2, 90],
>  
>          // Enforce curly brace conventions for all control statements.
> +        "curly": ["error", "all"],

This is the same setting as before. 2 is error and all is the default.

Comment 12

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089259 [details] [diff] [review]
part4-fix-eslint-disable-comments-0.patch

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

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +784,1 @@
>  upgrade.v2 = upgrade.v1 = function(db, version) {

Let's just put db in the list of acceptable identifiers.

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089260 [details] [diff] [review]
part5-fix-for-loop-0.patch

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

This loop should never have been written this way. Asking for trouble.

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089261 [details] [diff] [review]
part6-fix-useless-concats-0.patch

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

This patch makes me sad.

::: calendar/providers/storage/calStorageCalendar.js
@@ +1165,5 @@
>            "  AND recurrence_id_tz = :recurrence_id_tz"
>        );
>  
>        this.mSelectRecurrenceForItem = this.mDB.createStatement(
> +        "SELECT * FROM cal_recurrence WHERE item_id = :item_id AND cal_id = :cal_id"

This was written on two lines deliberately, but it'll do. I'd rather have it this way than a linting comment. :-/

::: calendar/test/unit/test_providers.js
@@ +4,5 @@
>  
>  var icalStringArray = [
>    // Comments refer to the range defined in testGetItems().
>    // 1: one-hour event
> +  // eslint-disable-next-line no-useless-concat

Just turn it off for the whole file, this is a mess.

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089258 [details] [diff] [review]
part3-reformat-code-0.patch

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

This patch won't apply to the tip when I apply it locally, so either it's not the one from your Try run or something weird is going on. It's all automated rewriting anyway and I'm okay with the process, so r+ based on that, with some comments you can fix in later patches.

::: calendar/.eslintrc.js
@@ +456,5 @@
> +      {
> +        min: 3,
> +        exceptions: [
> +          /* sorting */ "a",
> +          "b",

Ew. :-( I think the comments should go on their own lines.

::: calendar/base/modules/calRecurrenceUtils.jsm
@@ +64,5 @@
> +      "BYMINUTE",
> +      /* "BYDAY", */ "BYHOUR" /* "BYMONTHDAY", */,
> +      "BYYEARDAY",
> +      "BYWEEKNO",
> +      /* "BYMONTH", */ "BYSETPOS",

Ew.

::: calendar/base/modules/utils/calItemUtils.jsm
@@ +525,5 @@
> +      item.startDate = date;
> +      date = item.endDate.clone();
> +      date.addDuration(offset);
> +      item.endDate = date;
> +    } /* isToDo */ else {

I'm surprised this is allowed. The comment should probably be on the next line.

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +447,5 @@
> +      for (let update of zonesToUpdate) {
> +        executeSimpleSQL(
> +          db,
> +          "UPDATE cal_attendees    SET recurrence_id_tz = '" +
> +            update.newTzId +

Oh wow, what an improvement!

Can we rewrite these lines as template strings to stop this stupidity happening?

@@ +1364,5 @@
> +          transAlarm +
> +          "   FROM " +
> +          tblName +
> +          "  WHERE alarm_offset IS NOT NULL" +
> +          "     OR alarm_time IS NOT NULL;"

In fact
    a lot of
the SQL
    in this
        file is now a complete mess. Please can you tidy it up?

Comment 16

[Paul Morris [:pmorris]]

Thanks for the review. Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3e70e5eb53ed2e356a5b0b35677bb97080fe86d

Comment 17

[Paul Morris [:pmorris]]

Attachment: part1-turn-prettier-on-1.patch

1 of 7

Comment 18

[Paul Morris [:pmorris]]

Comment on attachment 9089389 [details] [diff] [review]
part1-turn-prettier-on-1.patch

This one did not get r+ yet, so requesting review.

Comment 19

[Paul Morris [:pmorris]]

Attachment: part2-adjust-settings-1.patch

2 of 7

This is the same setting as before. 2 is error and all is the default.

I changed it back to 2 to minimize changes. (Although FWIW I find ["error", "all"] more easily comprehensible).

Comment 20

[Paul Morris [:pmorris]]

Attachment: part3-reformat-code-1.patch

3 of 7

Comment 21

[Paul Morris [:pmorris]]

Attachment: part4-fix-eslint-disable-comments-1.patch

4 of 7

(In reply to Geoff Lankow (:darktrojan) from comment #12)

upgrade.v2 = upgrade.v1 = function(db, version) {

Let's just put db in the list of acceptable identifiers.

It was actually the .v1 and .v2 that were causing the linting error here. So I've left in the escape hatch comments for those.

Comment 22

[Paul Morris [:pmorris]]

Attachment: part5-fix-for-loop-1.patch

5 of 7

Comment 23

[Paul Morris [:pmorris]]

Attachment: part6-fix-useless-concats-1.patch

6 of 7

Just turn it off for the whole file, this is a mess.

Done.

Comment 24

[Paul Morris [:pmorris]]

Attachment: part7-improve-formatting-0.patch

7 of 7 This patch addresses some ugly results of the auto-formatting. I'm no expert in SQL syntax, but I tried to follow the previous formatting. This often involved a // prettier-ignore comment.

If a long multi-line string concatenation is a function argument, prettier wants to indent the lines after the first one to indicate that they are all part of the same function argument. But that gets in the way of being able to indent those following SQL norms.

Comment 25

[Paul Morris [:pmorris]]

Another try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e45a5591430cf926ded82fcbd2c7a71e4dac1bf4

Comment 26

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089389 [details] [diff] [review]
part1-turn-prettier-on-1.patch

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

Thanks.

Comment 27

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089402 [details] [diff] [review]
part7-improve-formatting-0.patch

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

That looks better.

FWIW template strings are multi-line, and SQL doesn't care about excess white-space, so you can
`just do
      this
      if you like.`
(I bet Bugzilla screws up this comment.)

Comment 28

[Paul Morris [:pmorris]]

Attachment: part1-turn-prettier-on-2.patch

Rebased.

Comment 29

[Paul Morris [:pmorris]]

Attachment: part2-adjust-settings-2.patch

Rebased.

Comment 30

[Paul Morris [:pmorris]]

Attachment: part3-reformat-code-2.patch

Rebased.

Comment 31

[Paul Morris [:pmorris]]

Attachment: part4-fix-eslint-disable-comments-2.patch

Rebased.

Comment 32

[Paul Morris [:pmorris]]

Attachment: part5-fix-for-loop-2.patch

Rebased.

Comment 33

[Paul Morris [:pmorris]]

Attachment: part6-fix-useless-concats-2.patch

Rebased.

Comment 34

[Paul Morris [:pmorris]]

Attachment: part7-improve-formatting-1.patch

Rebased.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 9089637 [details] [diff] [review]
part7-improve-formatting-1.patch

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

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +1773,5 @@
> +           "FROM cal_events AS e " +
> +          "WHERE e.id = cal_recurrence.item_id " +
> +            "AND e.cal_id = cal_recurrence.cal_id " +
> +          "UNION " +
> +         "SELECT t.flags " +

Nit: That UNION thing should be at the same level as the SELECT.
And the where should be intended by two, no? I see the original does one space

And above the " line up, and here they don't.

Comment 36

[Jorg K (CEST = GMT+2)]

Personally, I'd write it like this:

    executeSimpleSQL(db, "UPDATE cal_recurrence SET tmp_date_tz = " +
                           "(SELECT e.flags FROM cal_events AS e " +
                           "   WHERE e.id = cal_recurrence.item_id " +
                           "     AND e.cal_id = cal_recurrence.cal_id " +
                           " UNION SELECT t.flags FROM cal_todos AS t " +
                           "   WHERE t.id = cal_recurrence.item_id " +
                           "     AND t.cal_id = cal_recurrence.cal_id)");

Comment 37

[Paul Morris [:pmorris]]

Attachment: part7-improve-formatting-2.patch

Revised to follow most of Jorgk's nit suggestion for indenting that SQL string in calStorageUpgrade.jsm, but with some adjustments to match similar examples in the file (e.g. the use of // prettier-ignore).

Thanks for the timely reviews.

Comment 38

[Jorg K (CEST = GMT+2)]

I'll land it when get home in a few hours.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/790fe3dbbfe7
Turn on Prettier but ignore all directories. r=darktrojan
https://hg.mozilla.org/comm-central/rev/22c568242aac
Adjust eslint and prettier config for reformatting. r=darktrojan
https://hg.mozilla.org/comm-central/rev/53a946e22342
Reformat calendar code with eslint and Prettier. r=darktrojan
https://hg.mozilla.org/comm-central/rev/22388fb0b676
Reposition 'eslint-disable-' comments so they work. r=darktrojan
https://hg.mozilla.org/comm-central/rev/91339a81c040
Fix 'no-useless-concat' eslint errors. r=darktrojan
https://hg.mozilla.org/comm-central/rev/ba6c06d9ea3a
Improve some unfortunate auto-formatting. r=darktrojan
https://hg.mozilla.org/comm-central/rev/8e5b49be96e3
Edit a for loop to satisfy Prettier. r=darktrojan