Share and extend toolkit's eslint rules in PSM

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sfoster, Assigned: sfoster)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

The security/manager/.eslintrc.js has a lot of overlap with toolkit's .eslintrc.js. I'm proposing to directly extend toolkit's rules and remove the overlap so we start to converge on a common set of rules. Where PSM's rule are more explicit, we'll keep them in place. And fix any resulting lint errors under security/manager obviously.
Assignee: nobody → sfoster
Blocks: 1243179
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [psm-assigned]
A couple try runs: 

Try w. linux64 bit build, xpcshell and mochitest chrome tests:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c8b8d3cac82c64cf69396135562ff4662d534e5

Try win32/linux/macosx and PSM tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea5d71f14377abcef5bfeee88640cfbd63c95780

The change to mochitest.eslintrc.js especially may have an impact outside the security/manager component.
Attachment #8844047 - Flags: review?(jaws)
Comment on attachment 8844047 [details]
Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

https://reviewboard.mozilla.org/r/117602/#review119338

::: security/manager/.eslintrc.js:62
(Diff revision 1)
>      // Disallow modifying variables of class declarations.
>      "no-class-assign": "error",
>  
>      // Disallow assignment in conditional expressions, except if the assignment
>      // is within parentheses.
> -    "no-cond-assign": ["error", "except-parens"],
> +    // "no-cond-assign": ["error", "except-parens"],

Why does this rule get commented out? toolkit/.eslintrc.js has this rule enabled, except it doesn't use the 'except-parens' option.

Since this rule is commented out, it looks like it would use the toolkit version. Can we delete this line now?

::: security/manager/.eslintrc.js:175
(Diff revision 1)
> +  // "env": {
> +  //   "browser": true
> +  // },

Should this section be removed?

::: security/manager/pki/resources/content/device_manager.js:435
(Diff revision 1)
>  }
>  
>  // -------------------------------------   Old code
>  
> -function showTokenInfo()
> -{
> +function showTokenInfo() {
> +  // ClearInfoList();

We should just delete this line.

::: security/manager/ssl/tests/mochitest/mixedcontent/.eslintrc.js:6
(Diff revision 1)
>  "use strict";
>  
>  module.exports = { // eslint-disable-line no-undef
> -  "extends": "../../../../../../testing/mochitest/mochitest.eslintrc.js"
> +  "extends": "../../../../../../testing/mochitest/mochitest.eslintrc.js",
> +  "rules": {
> +    "brace-style": "off"

Can you add your commit message comment here saying why this rule is turned off?
Attachment #8844047 - Flags: review?(jaws) → review+
The addition of the plugin property in testing/mochitest/mochitest.eslintrc.js is already making its way to central in bug 1344690. I'll update this patch and attempt to land when that shows up on m-c.
Depends on: 1344690
Comment on attachment 8844047 [details]
Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

https://reviewboard.mozilla.org/r/117602/#review119338

> Why does this rule get commented out? toolkit/.eslintrc.js has this rule enabled, except it doesn't use the 'except-parens' option.
> 
> Since this rule is commented out, it looks like it would use the toolkit version. Can we delete this line now?

Yeah it needs deleting. I had a couple of commented blocks in this patch which I didn't finish deleting before pushing for review.
Comment on attachment 8844047 [details]
Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

https://reviewboard.mozilla.org/r/117602/#review119666

Got those nits taken care of, I think this is ready to go.
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93fa88dcfe7d
Extend toolkit's eslint rules and fix lint errors in PSM r=jaws
https://hg.mozilla.org/mozilla-central/rev/93fa88dcfe7d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.