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

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: standard8, Assigned: ahal)

Tracking

(Blocks: 1 bug)

3 Branch
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years 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

2 years 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

2 years 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

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

Updated

2 years ago
Blocks: 1295833
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1377049
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years 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

2 years ago
Attachment #8882768 - Flags: review?(standard8)
Attachment #8882638 - Flags: review?(standard8)
(Reporter)

Comment 14

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Duplicate of this bug: 1312081

Comment 22

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

Updated

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