1358041 - Clean up some browser/.eslintrc rules and fix a no-aArgs eslint warning in browser/extensions/formautofill/

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Chris Peterson [:cpeterson]]

This eslint warning is a regression from bug 1340468.

browser/extensions/formautofill/content/FormAutofillFrameScript.js
warning  Parameter 'aMessage' uses Hungarian Notation, consider using 'message' instead.  mozilla/no-aArgs (eslint)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1358041 - Fix no-aArgs eslint warning about Hungarian Notation in browser/extensions/formautofill/.

This eslint warning is a regression from bug 1340468.

browser/extensions/formautofill/content/FormAutofillFrameScript.js
warning  Parameter 'aMessage' uses Hungarian Notation, consider using 'message' instead.  mozilla/no-aArgs (eslint)

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1358041 - Remove browser/extensions/formautofill/.eslintrc rules already inherited from the "plugin:mozilla/recommended" eslint plugin.

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

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1358041 - Hoist some eslint rules from browser/ subdirectories to browser/.eslintrc.

All of browser/ JS code passes these eslint rules, so we can hoist them from subdirectories' .eslintrc to the common browser/.eslintrc.

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

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 8859908 [details]
Bug 1358041 - Hoist some eslint rules from browser/ subdirectories to browser/.eslintrc.

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

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8859906 [details]
Bug 1358041 - Fix no-aArgs eslint warning about Hungarian Notation in browser/extensions/formautofill/.

https://reviewboard.mozilla.org/r/131964/#review135230

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8859907 [details]
Bug 1358041 - Remove browser/extensions/formautofill/.eslintrc rules already inherited from the "plugin:mozilla/recommended" eslint plugin.

https://reviewboard.mozilla.org/r/131966/#review135232

::: commit-message-322cd:1
(Diff revision 1)
> +Bug 1358041 - Remove browser/extensions/formautofill/.eslintrc rules already inherited from the "plugin:mozilla/recommended" eslint plugin. r?standard8

There's a more comprehensive patch for doing this cleanup in bug 1354513. I'd expect that to be landing either over the weekend or early next week.

Could we hold off on this until then?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8859907 [details]
Bug 1358041 - Remove browser/extensions/formautofill/.eslintrc rules already inherited from the "plugin:mozilla/recommended" eslint plugin.

https://reviewboard.mozilla.org/r/131966/#review135234

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8859908 [details]
Bug 1358041 - Hoist some eslint rules from browser/ subdirectories to browser/.eslintrc.

https://reviewboard.mozilla.org/r/131968/#review135240

::: browser/.eslintrc.js:21
(Diff revision 2)
> +    // try { something(); } catch (e) { /* Silencing the error because ...*/ }
> +    // which is a valid use case.
> +    "no-empty": "error",
> +
> +    // No spaces between function name and parentheses
> +    "no-spaced-func": "error",

It looks like we can hoist this further into the recommended configuration - and apply to more of the tree.

If you can add it to tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js and bump the version number in tools/lint/eslint/eslint-plugin-mozilla/package.json, that would be even better.

::: browser/.eslintrc.js:24
(Diff revision 2)
> +
> +    // No spaces between function name and parentheses
> +    "no-spaced-func": "error",
> +
> +    // Maximum depth callbacks can be nested.
> +    "max-nested-callbacks": ["error", 8],

I wonder if we should put this in the recommended config instead. There's about 4 test files, where we could probably whitelist it, but otherwise most of the tree seems to be ok with it.

::: browser/.eslintrc.js:27
(Diff revision 2)
> +
> +    // Maximum depth callbacks can be nested.
> +    "max-nested-callbacks": ["error", 8],
> +
> +    // Disallow adding to native types
> +    "no-extend-native": "error",

This could possibly be hoisted to recommended, but maybe we should do that in a separate bug.

::: browser/.eslintrc.js:30
(Diff revision 2)
> +
> +    // Disallow adding to native types
> +    "no-extend-native": "error",
> +
> +    // No labels
> +    "no-labels": "error",

`no-labels` is already in the recommended configuration, so we don't need to add it here (unfortunately they aren't fully in order).

Comment 9

[Mark Banner (:standard8)]

The good news is bug 1354513 has just landed on autoland :-)

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Mark Banner (:standard8) from comment #8)
> It looks like we can hoist this further into the recommended configuration -
> and apply to more of the tree.
> 
> If you can add it to
> tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js and bump
> the version number in tools/lint/eslint/eslint-plugin-mozilla/package.json,
> that would be even better.

I had tried hoisting some of these rules into the recommended.js plugin, but the rules didn't seem to run then. Is that because I need to bump the package.json version every time I make local changes to recommended.js?

Comment 11

[Mark Banner (:standard8)]

(In reply to Chris Peterson [:cpeterson] from comment #10)
> I had tried hoisting some of these rules into the recommended.js plugin, but
> the rules didn't seem to run then. Is that because I need to bump the
> package.json version every time I make local changes to recommended.js?

The easier was is just to `./mach eslint --setup && ./mach eslint` each time. At some point soon I want to make that simpler.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22c3bc2c8ce6
Fix no-aArgs eslint warning about Hungarian Notation in browser/extensions/formautofill/. r=standard8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4df119a7e6
Hoist some eslint rules from browser/ subdirectories to browser/.eslintrc. r=standard8

Comment 13

[Chris Peterson [:cpeterson]]

Thanks! I'll look into consolidating the recommended.js changes in another bug.

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 8859907 [details]
Bug 1358041 - Remove browser/extensions/formautofill/.eslintrc rules already inherited from the "plugin:mozilla/recommended" eslint plugin.

This patch is obsolete because it is a duplicate of bug 1354513.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/22c3bc2c8ce6
https://hg.mozilla.org/mozilla-central/rev/9e4df119a7e6