Closed Bug 1361972 Opened 7 years ago Closed 7 years ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1361972 - [mozlint] Add ability to only lint staged changes to --workdir with git
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1361972 - Add a pre-push and pre-commit mozlint hooks
59 bytes, text/x-review-board-request
standard8
: review+
Details 
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.
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)
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)
Oh, git-cinnabar is bug 1369787.
Depends on: 1369787
Blocks: 1295833
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
Attachment #8882638 - Flags: review?(standard8)
Attachment #8882768 - Flags: review?(standard8)
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+
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+
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 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 :-)
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
https://hg.mozilla.org/mozilla-central/rev/55b72ade8f20
https://hg.mozilla.org/mozilla-central/rev/ea6fb7712a57
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: