Closed Bug 1442985 Opened 3 years ago Closed 3 years ago

Extend eslint coverage to previously preprocessed files

Categories

(Calendar :: General, enhancement)

Lightning 6.2
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

Attachments

(1 file, 3 obsolete files)

calendar/base/content/calendar-dnd-listener.js
calendar/base/content/dialogs/calendar-migration-dialog.js

have been previously preprocessed. since this is not the case anymore, these files should also be subject to linting now.
This patch takes care and apllies on top of patches from bug 393084, 1437405 and 463402 (in that order)
Attachment #8955893 - Flags: review?(philipp)
I borrowed your eslint user for this patch for better transparency when using blame.
Comment on attachment 8955893 [details] [diff] [review]
ExtendEslintCoverageForCalendar-V1.diff

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

This is going to bitrot on my calUtils patches, we should coordinate in what order these get pushed so we can adapt.

::: calendar/base/content/calendar-dnd-listener.js
@@ +320,5 @@
>                          return charset;
>                      },
>  
> +                    onStreamComplete: function(aLoader, context, status, unicharString) {
> +                        let parser = c["@mozilla.org/calendar/ics-parser;1"]

This should probably be Cc?
Attachment #8955893 - Flags: review?(philipp) → review+
Updated version with the comment considered.

Holding back checkin-needed at least until bug 463402 has landed.
Attachment #8955899 - Flags: review+
Is this ready to go?
Flags: needinfo?(makemyday)
If it still applies after the latest calutils consolidation checkin, it is. But I haven't crosschecked.
Flags: needinfo?(makemyday)
Attachment #8955893 - Attachment is obsolete: true
Debitrotted patch.

Philipp, I leave to your discretion whether to set chekin-needed now or wait until the other patches did land, since it's your patch queue that might get bitrotted. If you postpone it, feel free to debitrot then for checkin.
Attachment #8955899 - Attachment is obsolete: true
Attachment #8964185 - Flags: review+
I updated the patch to apply cleanly.
Attachment #8964185 - Attachment is obsolete: true
Attachment #8971938 - Flags: review+
Are you sure you want to land such a large patch without any test coverage? The tree rules say: No test, no landing :-(
Please present this again for landing when we have test coverage again, see bug 1457087.
Depends on: 1457087
Keywords: checkin-needed
Calendar Xpcshell tests are back, so let's see how this fares:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5c5504b30dc97a26b93c3194b3d8b025fa2a96fd

OK, looking at the changes, they are mostly cosmetic, so they should break anything, but better safe than sorry ;-)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0698244578e9
Extend eslint coverage for calendar. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 6.4
Attachment #8971938 - Flags: approval-calendar-beta?(philipp)
Attachment #8971938 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Please remove that flag again. This landed on Cal 6.4 currently in beta. There's nothing to uplift.

And, as requested before, grant me permission to set/reset those flags for administrative purposes :-(
Flags: needinfo?(philipp)
Flags: needinfo?(philipp)
Attachment #8971938 - Flags: approval-calendar-beta+
I've given you calendar-drivers access so you can reset flags, but please do not use it to grant approval unless it it test-only.
Thanks, will (not) do. As my aunt said: Use, but don't abuse ;-)
You need to log in before you can comment on or make changes to this bug.