Include the gdata provider in eslint

RESOLVED FIXED in 6.2

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Lightning 6.2

Details

Attachments

(1 attachment, 1 obsolete attachment)

We've been excluding the provider because of previous non-standard JS for Postbox compat. That is now gone, therefore we can include it again.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8948820 - Flags: review?(makemyday)
Try run looks comparable to c-c \o/

Comment 5

Last year
Comment on attachment 8948820 [details] [diff] [review]
Fix - v1

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

lgtm, r+ with the nits fixed.

::: calendar/providers/gdata/components/calGoogleCalendar.js
@@ +122,5 @@
>          if (aUri && aUri.schemeIs("googleapi")) {
>              // new format:  googleapi://session-id/?calendar=calhash@group.calendar.google.com&tasks=taskhash
>              let [fullUser, path] = aUri.pathQueryRef.substr(2).split("/", 2);
>              let parameters = new Map(path.substr(1).split("&").filter(Boolean)
> +                             .map(keyvalue => keyvalue.split("=", 2).map(decodeURIComponent)));

Can you rearrange this by moving the .map and maybe its argument to the line before and reduce the intent for the inner function?

@@ +793,2 @@
>                      cal.ERROR("[calGoogleCalendar] Error resetting calendar:\n" +
> +                            stringException(err));

Please increase the indent here by 2.

::: calendar/providers/gdata/content/browserRequest.js
@@ +63,5 @@
> +            this.securityButton.hidden = true;
> +            this.securityButton.removeAttribute("level");
> +        }
> +        this.securityButton.setAttribute("tooltiptext",
> +                                           browser.securityUI.tooltipText);

please fix the indent here.

::: calendar/providers/gdata/content/gdata-migration.js
@@ +86,5 @@
>          document.getElementById("showagain-checkbox").checked =
>              Preferences.get("calendar.google.migrate", true);
>      } else {
> +        // This is much more readable with lonely ifs
> +        /* eslint-disable no-lonely-if */

I wouldn't consider

if (Preferences.get("calendar.google.migrate", true) &&
    getMigratableCalendars().length > 0) {
...

really so much less readable here.

::: calendar/providers/gdata/modules/gdataUtils.jsm
@@ +264,5 @@
>   *
>   * @param dt        The calIDateTime to convert.
>   * @return          The RFC3339 string stamp.
>   */
> +function toRFC3339Fraction(date) {

still dt in docblock
Attachment #8948820 - Flags: review?(makemyday) → review+
Thanks for the review!

(In reply to [:MakeMyDay] from comment #5)
> ::: calendar/providers/gdata/components/calGoogleCalendar.js
> @@ +122,5 @@
> >          if (aUri && aUri.schemeIs("googleapi")) {
> >              // new format:  googleapi://session-id/?calendar=calhash@group.calendar.google.com&tasks=taskhash
> >              let [fullUser, path] = aUri.pathQueryRef.substr(2).split("/", 2);
> >              let parameters = new Map(path.substr(1).split("&").filter(Boolean)
> > +                             .map(keyvalue => keyvalue.split("=", 2).map(decodeURIComponent)));
> 
> Can you rearrange this by moving the .map and maybe its argument to the line
> before and reduce the intent for the inner function?
Done, I made use of extra variables and made sure it fits within 100 chars line length:

            let keyvalues = path.substr(1).split("&").filter(Boolean);
            let pairs = keyvalues.map(keyvalue => keyvalue.split("=", 2).map(decodeURIComponent));
            let parameters = new Map(pairs);

Normally I would put this into a helper function, but it is only used once.


> 
> I wouldn't consider
> 
> if (Preferences.get("calendar.google.migrate", true) &&
>     getMigratableCalendars().length > 0) {
> ...
> 
> really so much less readable here.

That is probably fine, it was actually more about the one level further up where else needs to be else if. I've changed it again and it doesn't look so bad after all, so I am removing the exception.
Posted patch Fix - v2 β€” β€” Splinter Review
Attachment #8948820 - Attachment is obsolete: true
Attachment #8950548 - Flags: review+

Comment 8

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f8afcb3f18de
Include the gdata provider in eslint. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 9

Last year
One hunk at https://hg.mozilla.org/comm-central/rev/f8afcb3f18de#l13.45 needed a rebase, apparently this line
  Services.prompt.alert(cal.window.getCalendarWindow(), title, quotaString);
had changed.
Target Milestone: --- → 6.2
You need to log in before you can comment on or make changes to this bug.