Closed Bug 1573670 Opened 2 years ago Closed 2 years ago

Wrap all custom element code in curly bracket blocks

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(3 files, 8 obsolete files)

9.22 KB, patch
Details | Diff | Splinter Review
25.74 KB, patch
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review

Most of our custom elements (classes etc.) are wrapped in curly bracket blocks to prevent leaking to window scope, but not all of them are. Wrap the remaining ones in blocks as well, and do this before the auto-formatting of bug 1572047 since it will affect indentation.

Part 1 of 3. Wrap all calendar CEs in curly brace blocks. Put "[skip-blame]" in the commit message to skip this change when doing hg blame since it's mostly re-indentation.

Attachment #9085447 - Flags: review?(mkmelin+mozilla)
Attachment #9085447 - Flags: review?(geoff)

Part 2 of 3. Wrap all TB CEs in curly brace blocks. Put "[skip-blame]" in the commit message to skip this change when doing hg blame since it's mostly re-indentation.

Attachment #9085448 - Flags: review?(mkmelin+mozilla)

Part 3 of 3. I found some redundant code at the bottom and the top of this file.

Try server run looks fine. Test failures seem to be unrelated:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=efa72012c8e6bbbd50308fc689ec65d662517b86

Attachment #9085449 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9085447 [details] [diff] [review]
part1-calendar-custom-elements-curly-blocks-0.patch

This looks fine based on a diff without white-space changes which I'm sure you did perfectly. ;-)
Attachment #9085447 - Flags: review?(geoff) → review+
Comment on attachment 9085447 [details] [diff] [review]
part1-calendar-custom-elements-curly-blocks-0.patch

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

Looks alright, though I thought the idea was to add the brackets and ignore the linting errors that would come from that, then check it in in the same go as the prettier run, so that prettier would take care of the re-indention where needed. Basically, the just a diff -uw of the changes here.
Attachment #9085447 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9085449 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9085448 [details] [diff] [review]
part2-tb-custom-elements-curly-blocks-0.patch

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

Same thing for this I gather. It would also bitrot in no time as is.

Hmm, I saw [skip-blame] in one of the patches. We need to research what the right thing is, for the C++ formatting I used # ignore-this-changeset. I think some of the source indexes ignore the latter under some circumstances.

mkmelin: Hm, sounds like I misunderstood. I was thinking it was an incremental step that could be done beforehand, to simplify the introduction of prettier. What are the reasons for waiting on this until the prettier run? (One benefit would be not having to do the indentation manually, but I guess I already did that...) I guess it complicates uplifting unless you apply it to the uplift targets, which creates more work.

Anyway, it sounds like you are saying I should redo these patches (using 'diff -uw') to remove the indentation changes, and then just apply them as part of the prettier run when that happens on Aug 29.

(In reply to Jorg K (GMT+2) from comment #7)

Hmm, I saw [skip-blame] in one of the patches. We need to research what the right thing is, for the C++ formatting I used # ignore-this-changeset. I think some of the source indexes ignore the latter under some circumstances.

I asked on IRC and we should indeed be using 'ignore-this-changeset', as described here (thanks Fallen):
https://lists.mozilla.org/pipermail/dev-platform/2019-January/023284.html

(In reply to Paul Morris [:pmorris] from comment #8)

Anyway, it sounds like you are saying I should redo these patches (using 'diff -uw') to remove the indentation changes, and then just apply them as part of the prettier run when that happens on Aug 29.

Yeah that would be what I suggest. Then there's no duplicate reformatting (if it's only indention).

Attachment #9085448 - Flags: review?(mkmelin+mozilla)

Part 1/3 - Now without indentation/whitespace changes and rebased on current trunk.
Previous version of this had darktrojan and mkmelin's r+.

Attachment #9085447 - Attachment is obsolete: true

Part 2/3 - Now without indentation/whitespace changes and rebased on current trunk.
Part 3/3 - Hasn't changed from the original part3 patch.

Attachment #9085448 - Attachment is obsolete: true
Attachment #9088497 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9088497 [details] [diff] [review]
part2-tb-custom-elements-curly-blocks-1.patch

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

Looks ok, though I think for the imports, you actually should use const ;)
Attachment #9088497 - Flags: review?(mkmelin+mozilla) → review+

1/3 Now with const instead of let for imports.

As you know, I'll gladly use const anywhere and anytime I can. :)

("If a variable is never reassigned, using the const declaration is better. const declaration tells readers, “this variable is never reassigned,” reducing cognitive load and improving maintainability." -- https://eslint.org/docs/rules/prefer-const )

Attachment #9088495 - Attachment is obsolete: true

2/3 Now with const instead of let for imports.

3/3

Attachment #9085449 - Attachment is obsolete: true
Attachment #9088497 - Attachment is obsolete: true
Blocks: 1577606
Blocks: 1577835

Rebased.

Attachment #9088722 - Attachment is obsolete: true

Rebased.

Attachment #9088723 - Attachment is obsolete: true

Rebased.

Attachment #9088724 - Attachment is obsolete: true
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1660b26d3a84
Wrap all calendar custom elements in curly bracket blocks. r=darktrojan,mkmelin
Keywords: leave-open

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fda41b757b2e
Wrap all Thunderbird custom elements in curly bracket blocks. r=mkmelin
https://hg.mozilla.org/comm-central/rev/6727b142f50d
Remove redundant menulist creation code. r=mkmelin

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Blocks: 1578477
You need to log in before you can comment on or make changes to this bug.