1436199 - Include the gdata provider in eslint

Status: RESOLVED FIXED

(Calendar :: Provider: GData, enhancement)

Description

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

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.

Comment 1

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

Attachment: Fix - v1

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c00e7e9a9b502881f9e836466c454fcacc732ee8

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9c6be5d1a69925a5f5495033919a98cc69e7e73f

Comment 4

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

Try run looks comparable to c-c \o/

Comment 5

[[:MakeMyDay]]

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=[e-mail]&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

Comment 6

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

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=[e-mail]&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.

Comment 7

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

Attachment: Fix - v2

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f8afcb3f18de
Include the gdata provider in eslint. r=MakeMyDay

Comment 9

[Jorg K (CEST = GMT+2)]

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.