1442985 - Extend eslint coverage to previously preprocessed files

Status: RESOLVED FIXED

Description

[[:MakeMyDay]]

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.

Comment 1

[[:MakeMyDay]]

Attachment: ExtendEslintCoverageForCalendar-V1.diff

This patch takes care and apllies on top of patches from bug 393084, 1437405 and 463402 (in that order)

Comment 2

[[:MakeMyDay]]

I borrowed your eslint user for this patch for better transparency when using blame.

Comment 3

[Philipp Kewisch [:Fallen] [:📆]]

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?

Comment 4

[[:MakeMyDay]]

Attachment: ExtendEslintCoverageForCalendar-V2.diff

Updated version with the comment considered.

Holding back checkin-needed at least until bug 463402 has landed.

Comment 5

[Philipp Kewisch [:Fallen] [:📆]]

Is this ready to go?

Comment 6

[[:MakeMyDay]]

If it still applies after the latest calutils consolidation checkin, it is. But I haven't crosschecked.

Comment 7

[[:MakeMyDay]]

Attachment: ExtendEslintCoverageForCalendar-V3.diff

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.

Comment 8

[Martin Schröder [:martinschroeder]]

Attachment: ExtendEslintCoverageForCalendar-V4.diff

I updated the patch to apply cleanly.

Comment 9

[Jorg K (GMT+2)]

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

[Jorg K (GMT+2)]

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

[Jorg K (GMT+2)]

Please present this again for landing when we have test coverage again, see bug 1457087.

Comment 12

[Jorg K (GMT+2)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0698244578e9
Extend eslint coverage for calendar. r=philipp

Comment 14

[Jorg K (GMT+2)]

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 :-(

Comment 15

[Philipp Kewisch [:Fallen] [:📆]]

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

[Jorg K (GMT+2)]

Thanks, will (not) do. As my aunt said: Use, but don't abuse ;-)