1348997 - mach eslint should not use the package.json version number to decide if the eslint-plugin-mozilla plugin needs to be updated

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Florian Quèze [:florian]]

Having to bump this version number in package.json causes bitrot whenever 2 developers are working on eslint rules at the same time. It's already a pain with 2-3 developers hacking rules, but will become unsustainable if we manage to convince more reviewers to write custom rules for review comments they made repeatedly.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

This should make it unnecessary to bump the version number of our self-built modules, by comparing the actual files.

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

Comment 2

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

https://reviewboard.mozilla.org/r/132874/#review135846

::: tools/lint/eslint.lint:156
(Diff revision 1)
> +def check_eslint_files(node_modules_path, name):
> +    def check_file_diffs(dcmp):
> +        # Diff files only looks at files that are different. Not for files
> +        # that are only present on one side. This should be generally OK as
> +        # new files will need to be added in the index.js for the package.
> +        if dcmp.diff_files and dcmp.diff_files != ['package.json']:

Why don't we care about package.json changes? Surely if the dependencies have changed we should re-install?

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

https://reviewboard.mozilla.org/r/132874/#review135846

> Why don't we care about package.json changes? Surely if the dependencies have changed we should re-install?

I'd excluded it because npm writes a lot more to the package.json that's installed into node_modules. However, you've made a very good point about the modules, so I'll see if I can adjust it to include something for that.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

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

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

https://reviewboard.mozilla.org/r/132874/#review135846

> I'd excluded it because npm writes a lot more to the package.json that's installed into node_modules. However, you've made a very good point about the modules, so I'll see if I can adjust it to include something for that.

I've now extended the script to compare the dependencies and devDependencies. I wasn't quite sure about the devDependencies list, but I thought it could be useful to have anyway.

Comment 6

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

https://reviewboard.mozilla.org/r/132874/#review136446

::: tools/lint/eslint.lint.py:204
(Diff revision 2)
> +            continue
> +
> +        if name == "eslint-plugin-mozilla" or name == "eslint-plugin-spidermonkey-js":
> +            # For our in-tree modules, check that package.json has the same dependencies.
> +            if (data["dependencies"] != expected_data["dependencies"] or
> +                data["devDependencies"] != expected_data["devDependencies"]):

You can't compare objects like this.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

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

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

https://reviewboard.mozilla.org/r/132874/#review136446

> You can't compare objects like this.

Ok, seems like python allows it for really simple dicts, but using cmp is better for most dicts, so I've switched to that.

Comment 9

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Comment on attachment 8860866 [details]
Bug 1348997 - When ESLint checks for reinstall, check the file diffs rather than a version number.

https://reviewboard.mozilla.org/r/132874/#review136888

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08cb2f95d844
When ESLint checks for reinstall, check the file diffs rather than a version number. r=mossop

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/08cb2f95d844

Comment 12

[Florian Quèze [:florian]]

Thank you very much for fixing this! It makes editing custom eslint rules so much nicer and easier! :-)