Change the eslint hooks to use ./mach lint --outgoing (or equivalent)

RESOLVED FIXED in Firefox 56

Status

Firefox Build System
Lint and Formatting
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: standard8, Assigned: ahal)

Tracking

(Blocks: 1 bug)

Version 3
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Now we have hooks for ESLint, I think it would be quite easy to extend them for flake8 (or add new ones). This would help python devs to see issues before they land.
(Assignee)

Comment 1

a year ago
Mozlint has a --outgoing parameter which is supposed to run all linters only against the files touched in commits that would get pushed to mozilla-central. It's supposed to work with git too, but someone recently reported trouble getting it working with git-cinnabar.

Assuming we iron out any remaining bugs with git-cinnabar, I'd love if instead of having a different hook for each linter, we just had a single hook that ran:
./mach lint --outgoing

I think doing this would also be simpler than the existing eslint hooks from a code perspective.

(Note: there are some potential rust and cpp linters coming down the pipeline soon, so using |mach lint --outgoing| would also automatically add those)
(Reporter)

Comment 2

a year ago
Ok, I think I'm happy with that idea, lets go with that for now, I think we can re-work this bug. I see the git-cinnabar issue as well, so I'll get that filed.
Assignee: standard8 → nobody
Summary: Add/Extend commit hooks for flake8 → Change the eslint hooks to use ./mach lint --outgoing (or equivalent)
(Reporter)

Comment 3

a year ago
Oh, git-cinnabar is bug 1369787.
Depends on: 1369787
(Reporter)

Updated

11 months ago
Blocks: 1295833
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1377049
Comment hidden (mozreview-request)
(Assignee)

Comment 6

11 months ago
This patch is almost ready to land, and should obsolete the eslintvalidate hooks (both git and hg). Instead of removing the old hooks, we could make them print an error message including the upgrade path to this new hook. I'd prefer to do this in a follow-up bug in a little while though (after these new hooks have had some more use).

Before landing this patch I need to:
* Support a mode for only linting staged changes on git
* Test the push hook on git (don't have cinnabar setup on this laptop)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8882768 - Flags: review?(standard8)
Attachment #8882638 - Flags: review?(standard8)
(Reporter)

Comment 14

11 months ago
mozreview-review
Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

https://reviewboard.mozilla.org/r/153876/#review159874

I'm a little concerned that we're not using the exact diffs according to the upstream we are actually pushing to, but I think this will be OK for the majority of cases.

One thing I did spot is that not each individual commit will necessarily pass lint - but I suspect that would be hard to enforce anyway, and not really much benefit given the current workflows.

So r=Standard8, there's just one bit that we do want to change to maintain consistency with the existing pre-commit hooks.

::: tools/lint/hooks.py:35
(Diff revision 3)
> +    return run_mozlint(hooktype, kwargs.get('pats', []))
> +
> +
> +def git(args=sys.argv[1:]):
> +    hooktype = os.path.basename(__file__)
> +    return run_mozlint(hooktype, args[:1])

I think returning the result in the 'push' case is correct - we want that to hard fail.

For the 'commit' case, I think we should always return success but just dumping the output (for both hooks).

See the discussion on bug 1360595 comment 6 / 7 and bug 1230300 comment 4.
Attachment #8882768 - Flags: review?(standard8) → review+
(Reporter)

Comment 15

11 months ago
mozreview-review
Comment on attachment 8882638 [details]
Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git

https://reviewboard.mozilla.org/r/153728/#review159876
Attachment #8882638 - Flags: review?(standard8) → review+
(Assignee)

Comment 16

11 months ago
mozreview-review-reply
Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

https://reviewboard.mozilla.org/r/153876/#review159874

Could you elaborate on the first part? If I run `hg push review`, then `review` is passed into the hook via the `pats` arg so the final command becomes `mach lint --outgoing review`. Similarly git will pass in the push destination via `sys.argv`. Have you noticed it linting against a different remote? If so, I'd like to fix that.

On the second point, yeah that would be difficult to implement. Plus the taskcluster tasks won't prevent that anyway, so seems not worth pursuing here.

> I think returning the result in the 'push' case is correct - we want that to hard fail.
> 
> For the 'commit' case, I think we should always return success but just dumping the output (for both hooks).
> 
> See the discussion on bug 1360595 comment 6 / 7 and bug 1230300 comment 4.

Makes sense!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 19

11 months ago
mozreview-review-reply
Comment on attachment 8882768 [details]
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks

https://reviewboard.mozilla.org/r/153876/#review159874

Aha, ok, I hadn't spotted the parts arg. So it probably will work as we want it to :-)

Comment 20

11 months ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55b72ade8f20
[mozlint] Add ability to only lint staged changes to --workdir with git r=standard8
https://hg.mozilla.org/integration/autoland/rev/ea6fb7712a57
Add a pre-push and pre-commit mozlint hooks r=standard8
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1312081

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55b72ade8f20
https://hg.mozilla.org/mozilla-central/rev/ea6fb7712a57
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

3 months ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.