Closed
Bug 1442985
Opened 7 years ago
Closed 7 years ago
Extend eslint coverage to previously preprocessed files
Categories
(Calendar :: General, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
6.4
People
(Reporter: MakeMyDay, Assigned: MakeMyDay)
References
Details
Attachments
(1 file, 3 obsolete files)
56.39 KB,
patch
|
mschroeder
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
This patch takes care and apllies on top of patches from bug 393084, 1437405 and 463402 (in that order)
Attachment #8955893 -
Flags: review?(philipp)
Assignee | ||
Comment 2•7 years ago
|
||
I borrowed your eslint user for this patch for better transparency when using blame.
Comment 3•7 years ago
|
||
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+
Assignee | ||
Comment 4•7 years ago
|
||
Updated version with the comment considered.
Holding back checkin-needed at least until bug 463402 has landed.
Attachment #8955899 -
Flags: review+
Assignee | ||
Comment 6•7 years ago
|
||
If it still applies after the latest calutils consolidation checkin, it is. But I haven't crosschecked.
Flags: needinfo?(makemyday)
Assignee | ||
Updated•7 years ago
|
Attachment #8955893 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
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+
Comment 8•7 years ago
|
||
I updated the patch to apply cleanly.
Attachment #8964185 -
Attachment is obsolete: true
Attachment #8971938 -
Flags: review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 9•7 years ago
|
||
Are you sure you want to land such a large patch without any test coverage? The tree rules say: No test, no landing :-(
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1ddf03790c090a4d45fabfc72ee1fd6b9458f3b6
Together with bug 1452120 and bug 1457087. At least that gives you Xpcshell coverage.
Comment 11•7 years ago
|
||
Please present this again for landing when we have test coverage again, see bug 1457087.
Depends on: 1457087
Keywords: checkin-needed
Comment 12•7 years ago
|
||
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 ;-)
Comment 13•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0698244578e9
Extend eslint coverage for calendar. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Target Milestone: --- → 6.4
Updated•7 years ago
|
Attachment #8971938 -
Flags: approval-calendar-beta?(philipp)
Updated•7 years ago
|
Attachment #8971938 -
Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Comment 14•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(philipp)
Attachment #8971938 -
Flags: approval-calendar-beta+
Comment 15•7 years ago
|
||
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.
Comment 16•7 years ago
|
||
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.
Description
•