Closed Bug 1325464 Opened 7 years ago Closed 7 years ago

Enable object-shorthand eslint rules for toolkit

Categories

(Toolkit :: General, defect)

Product:
Toolkit
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(3 files, 9 obsolete files)

Bug 1325464 - Use ES6 method syntax for preferences.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1325464 - Manually fix the remaining ES6 method syntax eslint errors.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
      No description provided.
Apologies if there are too many commits, I'm getting on the 'hg wip', 'hg histedit', 'firefoxtrees' train and off of the MQ train.
Comment on attachment 8821305 [details]
Bug 1325464 - Use ES6 method syntax for preferences.

https://reviewboard.mozilla.org/r/100582/#review101428
Attachment #8821305 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821306 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/100584/#review101430

::: browser/components/preferences/in-content/advanced.js:21
(Diff revision 1)
> -  init: function()
> +  init()
>    {

If you want to do another bug to move the braces to the same line I won't complain :)
Attachment #8821306 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821307 [details]
Bug 1325464 - Use ES6 method syntax for applications.js.

https://reviewboard.mozilla.org/r/100586/#review101432
Attachment #8821307 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821308 [details]
Bug 1325464 - Use ES6 method syntax for content.js.

https://reviewboard.mozilla.org/r/100588/#review101434
Attachment #8821308 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821309 [details]
Bug 1325464 - Use ES6 method syntax for main.js.

https://reviewboard.mozilla.org/r/100590/#review101436
Attachment #8821309 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821310 [details]
Bug 1325464 - Use ES6 method syntax for security.js.

https://reviewboard.mozilla.org/r/100592/#review101438
Attachment #8821310 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821311 [details]
Bug 1325464 - Use ES6 method syntax for subdialogs.js.

https://reviewboard.mozilla.org/r/100594/#review101440
Attachment #8821311 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821312 [details]
Bug 1325464 - Use ES6 method syntax for sync.js.

https://reviewboard.mozilla.org/r/100596/#review101442
Attachment #8821312 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821313 [details]
Bug 1325464 - Use ES6 method syntax for search.js.

https://reviewboard.mozilla.org/r/100598/#review101444
Attachment #8821313 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821314 [details]
Bug 1325464 - Use ES6 method syntax for privacy.js.

https://reviewboard.mozilla.org/r/100600/#review101446

::: browser/components/preferences/in-content/privacy.js:29
(Diff revision 1)
>  
>    /**
>     * Show the Tracking Protection UI depending on the
>     * privacy.trackingprotection.ui.enabled pref, and linkify its Learn More link
>     */
> -  _initTrackingProtection: function() {
> +  _initTrackingProtection() {

Please enforce this with eslint: http://eslint.org/docs/rules/object-shorthand

It even supports `--fix` so you can make sure you didn't miss anything.
Attachment #8821314 - Flags: review?(MattN+bmo) → review+
The last two patches I have uploaded are in response to comment #21. I have expanded the scope of this bug to fix all of /toolkit and /browser. https://reviewboard.mozilla.org/r/101196/diff/#index_header was auto-generated using --fix.
Summary: Use ES6 method definition syntax in /browser/components/preferences → Enable object-shortand eslint rules for toolkit
Summary: Enable object-shortand eslint rules for toolkit → Enable object-shorthand eslint rules for toolkit
Comment on attachment 8821306 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/100584/#review101430

> If you want to do another bug to move the braces to the same line I won't complain :)

Yeah, I'll fix that in another bug. Hopefully with an eslint rule to help keep us consistent.
Comment on attachment 8822216 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/101196/#review101896

This looks fine to me but I wonder if there should be a chance for people to voice why we should potentially wait on this as it may break some older tools (e.g. editor functionality to jump to a method definition?).

::: browser/base/content/aboutDialog-appUpdater.js:164
(Diff revision 2)
>        return Services.prefs.getBoolPref("app.update.auto");
>      }
>      catch (e) { }
>      return true; // Firefox default is true
>    },
>  

Your commit message seems to have two messages folded together which should be fixed. If you're using histedit you could have used `roll` instead of `fold` to avoid that.

::: toolkit/components/captivedetect/captivedetect.js:338
(Diff revision 2)
>      };
>  
>      this._runningRequest['urlFetcher'] = urlFetcher;
>    },
>  
>    _startLogin: function _startLogin() {

`--fix` understandably doesn't seem to touch cases where the function name matches the property name like this so it's possible that this patch is introducing inconsitency of shorthand vs. longhand with a function name in the same object but in those cases there was already inconsitency of longhand without vs. with a function name so I think that's okay.
Attachment #8822216 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8822217 [details]
Bug 1325464 - Manually fix the remaining ES6 method syntax eslint errors.

https://reviewboard.mozilla.org/r/101198/#review101900
Attachment #8822217 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8822216 [details]
Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled.

https://reviewboard.mozilla.org/r/101196/#review101896

SublimeText3 supports it and I see that VisualStudio also supports it. I'm not sure what other tools people may be using, but we've had usages of it in our code since they were introduced around Firefox33/34 so I'm OK with pushing this forward.
Attachment #8821307 - Attachment is obsolete: true
Attachment #8821308 - Attachment is obsolete: true
Attachment #8821309 - Attachment is obsolete: true
Attachment #8821310 - Attachment is obsolete: true
Attachment #8821311 - Attachment is obsolete: true
Attachment #8821312 - Attachment is obsolete: true
Attachment #8821313 - Attachment is obsolete: true
Attachment #8821314 - Attachment is obsolete: true
Attachment #8822216 - Attachment is obsolete: true
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4079437c4648
Use ES6 method syntax for preferences. r=MattN
https://hg.mozilla.org/integration/autoland/rev/cd10db6087dd
Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled. r=MattN
https://hg.mozilla.org/integration/autoland/rev/562ddc32cc21
Manually fix the remaining ES6 method syntax eslint errors. r=MattN
Component: Preferences → General
Product: Firefox → Toolkit
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01d6a34ee366
Use ES6 method syntax for preferences. r=MattN
https://hg.mozilla.org/integration/autoland/rev/15ea2e33fcd4
Enable object-shorthand rule and run 'mach eslint --fix' with the rule enabled. r=MattN
https://hg.mozilla.org/integration/autoland/rev/e551e215986a
Manually fix the remaining ES6 method syntax eslint errors. r=MattN
Flags: needinfo?(jaws)
Depends on: 1326305
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> Comment on attachment 8821306 [details]
> Bug 1325464 - Enable object-shorthand rule and run 'mach eslint --fix' with
> the rule enabled.
> 
> https://reviewboard.mozilla.org/r/100584/#review101430
> 
> > If you want to do another bug to move the braces to the same line I won't complain :)
> 
> Yeah, I'll fix that in another bug. Hopefully with an eslint rule to help
> keep us consistent.

Filed bug 1326511 for this work.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: