Closed Bug 1577606 Opened 2 years ago Closed 2 years ago

Use Prettier for formatting JavaScript in Calendar

Categories

(Calendar :: General, task, P3)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(7 files, 14 obsolete files)

1.38 KB, patch
Details | Diff | Splinter Review
3.93 KB, patch
Details | Diff | Splinter Review
6.91 MB, patch
Details | Diff | Splinter Review
9.01 KB, patch
Details | Diff | Splinter Review
1.32 KB, patch
Details | Diff | Splinter Review
23.56 KB, patch
Details | Diff | Splinter Review
25.10 KB, patch
Details | Diff | Splinter Review

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).

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.

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?

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.

Attached patch part1-turn-prettier-on-0.patch (obsolete) — Splinter Review

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.

Attachment #9089256 - Flags: review?(geoff)
Attached patch part2-adjust-settings-0.patch (obsolete) — Splinter Review

2 of 6. Do settings before reformatting.

Attachment #9089257 - Flags: review?(geoff)
Attached patch part3-reformat-code-0.patch (obsolete) — Splinter Review

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|.

Attachment #9089258 - Flags: review?(geoff)

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.

Attachment #9089259 - Flags: review?(geoff)
Attached patch part5-fix-for-loop-0.patch (obsolete) — Splinter Review

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

Attachment #9089260 - Flags: review?(geoff)

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.

Attachment #9089261 - Flags: review?(geoff)
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 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.
Attachment #9089257 - Flags: review?(geoff) → review+
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.
Attachment #9089259 - Flags: review?(geoff) → review+
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.
Attachment #9089260 - Flags: review?(geoff) → review+
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.
Attachment #9089261 - Flags: review?(geoff) → review+
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?
Attachment #9089258 - Flags: review?(geoff) → review+
Attached patch part1-turn-prettier-on-1.patch (obsolete) — Splinter Review

1 of 7

Attachment #9089256 - Attachment is obsolete: true
Attachment #9089256 - Flags: review?(geoff)
Comment on attachment 9089389 [details] [diff] [review]
part1-turn-prettier-on-1.patch

This one did not get r+ yet, so requesting review.
Attachment #9089389 - Flags: review?(philipp)
Attachment #9089389 - Flags: review?(geoff)
Attached patch part2-adjust-settings-1.patch (obsolete) — Splinter Review

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).

Attachment #9089257 - Attachment is obsolete: true
Attached patch part3-reformat-code-1.patch (obsolete) — Splinter Review

3 of 7

Attachment #9089258 - Attachment is obsolete: true

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.

Attachment #9089259 - Attachment is obsolete: true
Attached patch part5-fix-for-loop-1.patch (obsolete) — Splinter Review

5 of 7

Attachment #9089260 - Attachment is obsolete: true

6 of 7

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

Done.

Attachment #9089261 - Attachment is obsolete: true
Attached patch part7-improve-formatting-0.patch (obsolete) — Splinter Review

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.

Attachment #9089402 - Flags: review?(philipp)
Attachment #9089402 - Flags: review?(geoff)
Blocks: 1577835
Comment on attachment 9089389 [details] [diff] [review]
part1-turn-prettier-on-1.patch

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

Thanks.
Attachment #9089389 - Flags: review?(philipp)
Attachment #9089389 - Flags: review?(geoff)
Attachment #9089389 - Flags: review+
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.)
Attachment #9089402 - Flags: review?(philipp)
Attachment #9089402 - Flags: review?(geoff)
Attachment #9089402 - Flags: review+

Rebased.

Attachment #9089389 - Attachment is obsolete: true

Rebased.

Attachment #9089394 - Attachment is obsolete: true

Rebased.

Attachment #9089395 - Attachment is obsolete: true

Rebased.

Attachment #9089397 - Attachment is obsolete: true

Rebased.

Attachment #9089398 - Attachment is obsolete: true

Rebased.

Attachment #9089399 - Attachment is obsolete: true
Attached patch part7-improve-formatting-1.patch (obsolete) — Splinter Review

Rebased.

Attachment #9089402 - Attachment is obsolete: true
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.

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)");

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.

Attachment #9089637 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed

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

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 70
Blocks: 1578477
You need to log in before you can comment on or make changes to this bug.