Status

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

unspecified

Details

Attachments

(17 attachments, 4 obsolete attachments)

202.51 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
5.48 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.56 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.85 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.41 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.98 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.36 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
9.45 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.26 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.24 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
28.75 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
3.31 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
2.70 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.18 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
1.15 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
10.12 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
26.74 KB, application/octet-stream
Details
There have been some changes to the toolkit eslint configs that trigger errors in calendar, there are some new rules and likely also some minor issues in our codebase.

I am fully aware that we should be running these tests with the build, but until we swap around the build system I think this will be unlikely to reasonably work. Maybe I should just run builds on my own CI for the meanwhile.

A few notable changes:
* In calendar/base/content/dialogs/calendar-event-dialog-recurrence.js I removed the appendChild/removeChild combo, using appendChild should removeChild automatically (needed due to mozilla/avoid-removeChild rule)
* I needed to change formatting in test_recur.js. It wasn't pretty before, and it doesn't look better now. Sorry about that.
* In calendar/base/content/import-export.js I switched around the if blocks to remove an error
* In calendar/base/content/preferences/alarms.js I switched for(;;) to for..of
* The no-useless-escape rule fired for various regular expressions, which I am a bit wary of. I disabled the rule for backslash in a character group because this breaks syntax highlighting in my editor. Unfortunately no way to configure this rule.
* calendar/base/modules/calItipUtils.jsm had an unused variable replyer. You might want to check if this is a looming bug or not.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8846649 - Flags: review?(makemyday)
Posted patch Fix - v1 (no whitespaces) (obsolete) β€” β€” Splinter Review
This one might be easier to review, it is a no whitespace diff.
You don't need to create a separate patch without whitespace changes, but splitting up the big patch in multiple smaller ones would be appreciated, so the review can be done one by one.
Splitting this up after the fact would probably be more effort than reviewing it in one run. Do you think you could do the review on the full file? It is far not as long as the other eslint changes :)
Flags: needinfo?(makemyday)
I can do the review that way, but you would have to live with incremental review segements that way (most probably file based).
Flags: needinfo?(makemyday)
Works for me, sorry for the inconvenience!
Priority: -- → P3
Comment on attachment 8846649 [details] [diff] [review]
Fix - v1

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

Thanks. Just a few comments, please see below - r+ with that fixed. In general, no curly brackets on case blocks please.

Hopefully, the patch is not too bitrotted, since it took some time to get to it. At least the recent datetimeformatter changes will probably require some updates. Please make a try run before landing this to make sure the tests are still working. I don't like parts of the test code reformatting in test_ltninvitationutils - even if the previous formatting wasn't really pretty, it was consistent - now, it's probably neither.

::: calendar/base/modules/calItipUtils.jsm
@@ -301,5 @@
>                                      item.getAttendees({}),
>                                      itipItem.sender
>                              );
>                              if (attendees.length == 1) {
> -                                let replyer = foundItems[0].getAttendeeById(attendees[0].id);

This is indead strange - I'll take a look at this separately.

::: calendar/base/modules/calRecurrenceUtils.jsm
@@ +63,5 @@
> +            "BYYEARDAY",
> +            "BYWEEKNO",
> +            // "BYMONTH",
> +            "BYSETPOS"
> +        ];

Can you please spare some lines here by wrapping two or three items in a line?

::: calendar/lightning/content/lightning-item-iframe.js
@@ +1258,5 @@
>          if (rules.length == 1) {
>              let rule = cal.wrapInstance(rules[0], Components.interfaces.calIRecurrenceRule);
>              if (rule) {
>                  switch (rule.type) {
> +                    case "DAILY": {

Please don't add curly brackets for the case blocks - here and for the other occurrences in this file.

::: calendar/lightning/modules/ltnInvitationUtils.jsm
@@ +38,5 @@
>                                             [organizerString, summary]);
>                      break;
>                  case "COUNTER":
>                      // falls through
> +                case "REPLY": {

Please don't use curly brackets for case blocks.

::: calendar/test/unit/test_ltninvitationutils.js
@@ +985,4 @@
>                      organizer: "ORGANIZER;RSVP=TRUE;CN=Organizer;PARTSTAT=ACCEPTED;ROLE=CHAI" +
>                                 "R:mailto:organizer2@example.net"
> +                },
> +                {

Please leave it with }, { in such specific cases.
Attachment #8846649 - Flags: review?(makemyday) → review+
Duplicate of this bug: 1383995
Here is an updated patch that matches Fix v1, with comments taken into account. I disabled object-curly-newline to make that data block consistent. Note I didn't fix the case block issue, since that block is "required" due to the defined variables.
Attachment #8846649 - Attachment is obsolete: true
Attachment #8846651 - Attachment is obsolete: true
Attachment #8891837 - Flags: review+
Attachment #8891839 - Flags: review?(makemyday)
Attachment #8891839 - Attachment description: Fix id-length rule - v1 → Part 02 - Fix id-length rule - v1
no-eval is one I am not sure about. I believe using JSON will work just as well, but I don't know for sure.
Attachment #8891843 - Flags: review?(makemyday)
Attachment #8891846 - Flags: review?(makemyday)
Attachment #8891847 - Flags: review?(makemyday)
Posted patch Part 13 - Fix indent rule - v1 (obsolete) β€” β€” Splinter Review
Note if testing this one locally, you have to make sure you change the xbl preprocessor in eslint-plugin-mozilla to 4 space indent. Bugs are filed to remove this step.
Attachment #8891850 - Flags: review?(makemyday)
Those should be all for now, with all those applied I get zero errors/warnings. Here is a try run, which looks good afaict.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d83146e0306fb14dbf6822f399cf7632febb334f
Attachment #8891837 - Attachment description: Previous Patch - v2 → Part 01 - Previous Patch - v2
Attachment #8891839 - Flags: review?(makemyday) → review+
Attachment #8891840 - Flags: review?(makemyday) → review+
Attachment #8891842 - Flags: review?(makemyday) → review+
Attachment #8891844 - Flags: review?(makemyday) → review+
Attachment #8891845 - Flags: review?(makemyday) → review+
Attachment #8891846 - Flags: review?(makemyday) → review+
Attachment #8891847 - Flags: review?(makemyday) → review+
Attachment #8891848 - Flags: review?(makemyday) → review+
Attachment #8891850 - Flags: review?(makemyday) → review+
Attachment #8891849 - Flags: review?(makemyday) → review+
Comment on attachment 8891843 [details] [diff] [review]
Part 06 - Fix no-eval rule - v1

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

I did some tests and this seems to be a reasonable replacement here, so r+
Attachment #8891843 - Flags: review?(makemyday) → review+
Comment on attachment 8891841 [details] [diff] [review]
Part 04 - Fix use-default-preference-values rule - v1

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

r+ with the comment below considered.

::: calendar/base/content/preferences/categories.js
@@ +146,5 @@
>       */
>      editCategory: function() {
>          let list = document.getElementById("categorieslist");
>          let categoryNameFix = cal.formatStringForCSSRule(gCategoryList[list.selectedIndex]);
> +        let currentColor = categoryPrefBranch.getCharPref(categoryNameFix, "");

If switching from null to an empty string here, you probably need to adopt the interpreter of the color param here:

https://dxr.mozilla.org/comm-central/source/calendar/base/content/preferences/editCategory.js#18
Attachment #8891841 - Flags: review?(makemyday) → review+
Good catch, thanks! I've changed it to !! to force it to boolean, which catches null and empty string.
Attachment #8916263 - Flags: review+
Attachment #8891841 - Attachment is obsolete: true
Just one whitespace more in v2, carrying over r+
Attachment #8891850 - Attachment is obsolete: true
Attachment #8916270 - Flags: review+
MakeMyDay: if there are no issues in the review, please set checkin-needed so we can get this out.

JΓΆrg: I'm attaching a hg bundle with patches in the right order for your convenience, I can also just push them myself if you ping me with a time that is convenient for you.

The bundle should contain 16 changesets with these parts. I've never manually created a bundle like this so please check |hg out| before pushing.
Attachment #8916264 - Flags: review?(makemyday) → review+
Attachment #8916266 - Flags: review?(makemyday) → review+
Comment on attachment 8916269 [details] [diff] [review]
Part 16 - Fix mozilla/no-useless-run-test rule - v1

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

Looks good all together, thanks for keeping up with that.
Attachment #8916269 - Flags: review?(makemyday) → review+
Keywords: checkin-needed
Comment on attachment 8891837 [details] [diff] [review]
Part 01 - Previous Patch - v2

The first part " Part 01 - Previous Patch - v2" already fails to apply:
$ hg qpush
applying eslint-fixes.diff
patching file calendar/.eslintrc.js
Hunk #1 FAILED at 0
1 out of 5 hunks FAILED -- saving rejects to file calendar/.eslintrc.js.rej
patching file calendar/base/content/today-pane.js
Hunk #1 FAILED at 19
1 out of 4 hunks FAILED -- saving rejects to file calendar/base/content/today-pane.js.rej
patching file calendar/providers/caldav/calDavCalendar.js
Hunk #3 FAILED at 2196
1 out of 3 hunks FAILED -- saving rejects to file calendar/providers/caldav/calDavCalendar.js.rej
patching file calendar/providers/ics/calICSCalendar.js
Hunk #1 FAILED at 680
1 out of 3 hunks FAILED -- saving rejects to file calendar/providers/ics/calICSCalendar.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh eslint-fixes.diff

2-10 applied, 11 failed:
$ hg qpush
applying 11-eslint-space-before-function-parens.diff
patching file calendar/test/unit/test_ltninvitationutils.js
Hunk #1 FAILED at 1066
1 out of 1 hunks FAILED -- saving rejects to file calendar/test/unit/test_ltninvitationutils.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 11-eslint-space-before-function-parens.diff

12 and 13 failed, I'll spare you the details. 14-16 work again.
Are you saying the bundle can be applied? How?
Flags: needinfo?(philipp)
Keywords: checkin-needed
Looks like I did more to keep the first parts up to date than I was aware of. This should do it:

hg unbundle eslint.hg
hg out -r . comm # recommended

find me on IRC if you need help!
Flags: needinfo?(philipp) → needinfo?(jorgk)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/076da1348bcc
Fix calendar eslint issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/69ed6b20f6f2
Fix calendar eslint issues - Fix id-length issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/b97e6e08645d
Fix calendar eslint issues -  Fix mozilla/avoid-removeChild issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/1ab10b6c3b56
Fix calendar eslint issues - Fix mozilla/use-default-preference-values issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/19af38e2d4b1
Fix calendar eslint issues - Fix mozilla/var-only-at-top-level issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/938e0676a3cd
Fix calendar eslint issues - Fix no-eval issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/72d338633595
Fix calendar eslint issues - Fix no-negated-condition issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/a63c88905470
Fix calendar eslint issues - Fix no-unused-vars issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/f3a5abbda9c1
Fix calendar eslint issues - Fix quotes issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/ca289424ad65
Fix calendar eslint issues - Fix semi issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/ec5ea6aa64c2
Fix calendar eslint issues - Fix space-before-function-paren issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/323aa523a4b0
Fix calendar eslint issues - Fix mozilla/no-useless-parameters issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/96bcae4b9bbc
Fix calendar eslint issues - Fix indent issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/d038bfb4ca16
Fix calendar eslint issues - "Fix" no-unsanitized/property issues. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/34e4b127cd10
Fix calendar eslint issues - Fix comma-spacing issue. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/86066f04ca11
Fix calendar eslint issues - Fix mozilla/no-useless-run-test issues. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Just for the record, that was a bit of a fight with HG.

hg unbundle eslint.hg
restores the changesets as a branch to where you started off, in this case from 54b88d31ffab (Thu Oct 5, 2017, 11:09:39).

To move the 16 changesets in the branch onto the default branch I had to
hg rebase -b <first changeset in that bundle branch>. That gave

rebasing 22297:eb5e2e1df6f4 "Bug 1346797 - Fix calendar eslint issues. r=MakeMyDay"
merging calendar/base/modules/calUtils.jsm
merging calendar/providers/storage/calStorageCalendar.js
rebasing 22298:eceb0a77d9e6 "Bug 1346797 - Fix calendar eslint issues - Fix id-length issues. r=MakeMyDay"
...
rebasing 22312:ee4d5e948f5f "Bug 1346797 - Fix calendar eslint issues - Fix mozilla/no-useless-run-test issues. r=MakeMyDay" (tip)

|hg unbundle| actually suggested merge and commit, but that didn't give the correct graph when looking at it with TortoiseHg.
Flags: needinfo?(jorgk)
Target Milestone: --- → 6.0
Duplicate of this bug: 1386804
You need to log in before you can comment on or make changes to this bug.