1431104 - Add lint rule to force use of ChromeUtils over Cu.import (turn off by default)

Status: RESOLVED DUPLICATE of bug 1431533

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[:Gijs (he/him)]

As part of our switch it'd be helpful if the old thing wasn't reintroduced. We'll likely need to keep it around for a bit because of system and distribution add-ons, but even so.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1431104 - add lint rule for use of ChromeUtils.import over Cu.import,

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8943271 [details]
Bug 1431104 - add lint rule for use of ChromeUtils.import over Cu.import,

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

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8943271 [details]
Bug 1431104 - add lint rule for use of ChromeUtils.import over Cu.import,

https://reviewboard.mozilla.org/r/213596/#review219688

The rule looks good, but we should update the docs and tests as well.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-import.js:2
(Diff revision 2)
> +/**
> + * @fileoverview Require use of ChromeUtils.import instead of Cu/Components.utils.import

Please can you add a short doc for this in case anyone looks it up. See tools/lint/docs/linters/eslint-plugin-mozilla.rst for our existing docs, just add a section there.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-import.js:14
(Diff revision 2)
> +"use strict";
> +
> +// -----------------------------------------------------------------------------
> +// Rule Definition
> +// -----------------------------------------------------------------------------
> +module.exports = function(context) {

Please can you add a test for this, basing it around test-no-single-arg-cu-import.js should give you a good start.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-import.js:43
(Diff revision 2)
> +      } else {
> +        return;
> +      }
> +
> +      context.report(node,
> +                     `Use ChromeUtils.import rather than Components.utils.import().`);

nit: Add brackets after the first import.

Comment 4

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #1)
> Created attachment 8943271 [details]
> Bug 1431104 - add lint rule for use of ChromeUtils.import over Cu.import,
> 
> Review commit: https://reviewboard.mozilla.org/r/213596/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/213596/

It looks like we wrote more or less the same rule. Sorry about that. I added this to the rule I wrote for bug 1431533 before I realized you were already working on the ESLint part.

If you want, I can remove that part of my rule and we can merge after the rest of those patches get reviewed.

Comment 5

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #4)
> (In reply to :Gijs from comment #1)
> > Created attachment 8943271 [details]
> > Bug 1431104 - add lint rule for use of ChromeUtils.import over Cu.import,
> > 
> > Review commit: https://reviewboard.mozilla.org/r/213596/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/213596/
> 
> It looks like we wrote more or less the same rule. Sorry about that. I added
> this to the rule I wrote for bug 1431533 before I realized you were already
> working on the ESLint part.
> 
> If you want, I can remove that part of my rule and we can merge after the
> rest of those patches get reviewed.

Nah, your stuff looks good and you have a --fix implementation. I expect Florian will review your work today, so I will just dupe this over.