1344308 - Share and extend toolkit's eslint rules in PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Sam Foster [:sfoster] (he/him)]

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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

* Remove eslint rules for PSM which are redundant with toolkit/.eslintrc.js
* Fix missing plugins block in mochitest.eslintrc.js
* Disable brace-style checking in mixed-content mochitests which all use the same boilerplate
* Fix lint errors resulting from new rules defined by toolkit's eslintrc.js

Review commit: https://reviewboard.mozilla.org/r/117602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117602/

Comment 2

[Sam Foster [:sfoster] (he/him)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 4

[Sam Foster [:sfoster] (he/him)]

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.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8844047 [details]
Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117602/diff/1-2/

Comment 6

[Sam Foster [:sfoster] (he/him)]

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 7

[Sam Foster [:sfoster] (he/him)]

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.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8844047 [details]
Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117602/diff/2-3/

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8844047 [details]
Bug 1344308 - Extend toolkit's eslint rules and fix lint errors in PSM

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117602/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93fa88dcfe7d
Extend toolkit's eslint rules and fix lint errors in PSM r=jaws

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93fa88dcfe7d