Add a git pre-commit hook to run ESLint

RESOLVED FIXED in Firefox 55

Status

Testing
Lint
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks: 1 bug)

Version 3
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

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

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
We already have a commit hook for Mercurial, so we should add one for git as well as people like to use git-cinnabar.

I've put together a hook that re-uses code from the Mercurial hook, so that we're duplicating less.

To set it up locally, run in the top-level of the repo (assuming no existing pre-commit hooks):

ln -s ../../tools/git/git-lint-commit-hook.sh .git/hooks/pre-commit
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

7 months ago
mozreview-review
Comment on attachment 8862909 [details]
Bug 1360595 - Make the ESLint Mercurial pre-commit hook be run in and pass flake8 validation.

https://reviewboard.mozilla.org/r/134782/#review138162
Attachment #8862909 - Flags: review?(dtownsend) → review+

Comment 4

7 months ago
mozreview-review
Comment on attachment 8862910 [details]
Bug 1360595 - Add a git pre-commit hook for running ESLint.

https://reviewboard.mozilla.org/r/134784/#review138182

I think we should move the shared functions out into their own file in tools/lint/eslint.

::: tools/git/eslintvalidategit.py:13
(Diff revision 1)
> +from eslintvalidate import is_lintable, runESLint
> +
> +
> +# This Mocks the `ui` object that Mercurial passes to the eslintvalidate
> +# commands.
> +class UIMock(object):

I think it would be better to have the runESLint function accept a print function then passing ui.warn from the mercurial side for that.

::: tools/git/eslintvalidategit.py:41
(Diff revision 1)
> +
> +if __name__ == '__main__':
> +    if (eslint()):
> +        sys.exit(0)
> +
> +    sys.exit(1)

With the mercurial hook we don't abort the commit if there aren't errors but just display the errors. This allows users to keep working on changes. The git hook should work the same.
Attachment #8862910 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 5

7 months ago
(In reply to Dave Townsend [:mossop] from comment #4)
> ::: tools/git/eslintvalidategit.py:41
> (Diff revision 1)
> > +
> > +if __name__ == '__main__':
> > +    if (eslint()):
> > +        sys.exit(0)
> > +
> > +    sys.exit(1)
> 
> With the mercurial hook we don't abort the commit if there aren't errors but
> just display the errors. This allows users to keep working on changes. The
> git hook should work the same.

Hmm, personally I'm not overkeen on that approach as I find it confusing when it outputs an error, but then lets the commit happen anyway.

git does have a --no-verify argument which allows you to skip the hook if you really want to.

Unfortunately the best in Mercurial is `hg --config hooks.pre-commit= commit`


So I'd lean to either:

- Print message about how to skip & exit 1
- Print message about the commit happening anyway and exit 0

We could do the former for git, and the latter for hg. Though I guess it would probably make sense for them to be the same.

Thoughts?
Flags: needinfo?(dtownsend)
(Assignee)

Updated

7 months ago
Blocks: 1295833
So the rationale for what to do originally came from gps in bug 1230300 comment 4 so I wouldn't want to change the mercurial commit. I don't have strong feelings on the git behaviour since I don't use git but I think gps's argument stands so I'd prefer to see consistency and just print a warning.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 7

7 months ago
(In reply to Dave Townsend [:mossop] from comment #6)
> So the rationale for what to do originally came from gps in bug 1230300
> comment 4 so I wouldn't want to change the mercurial commit. I don't have
> strong feelings on the git behaviour since I don't use git but I think gps's
> argument stands so I'd prefer to see consistency and just print a warning.

Thank you for that reference. That makes some sense, so lets go with that for now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

7 months ago
mozreview-review
Comment on attachment 8862910 [details]
Bug 1360595 - Add a git pre-commit hook for running ESLint.

https://reviewboard.mozilla.org/r/134784/#review138940

::: tools/git/eslintvalidate.py:22
(Diff revision 2)
> +
> +    files = []
> +
> +    for file in f.read().splitlines():
> +        if is_lintable(file):
> +            files.append(file)

files = [file for file in f.read().splitlines() if is_lintable(file)]

::: tools/git/eslintvalidate.py:41
(Diff revision 2)
> +    output("Note: ESLint failed, but the commit will still happen. "
> +           "Please fix before pushing.")
> +    # We output successfully as that seems to be a better model. See
> +    # https://bugzilla.mozilla.org/show_bug.cgi?id=1230300#c4 for more
> +    # information.
> +    sys.exit(0)

This would be a littel cleaner if you just output on not eslint() then you wouldn't need the sys.exit calls at all.

::: tools/lint/eslint/hook_helper.py:46
(Diff revision 2)
> -    ctx = repo[node]
> -    if len(ctx.parents()) > 1:
> -        return 0
>  
> -    deleted = repo.status(ctx.p1().node(), ctx.node()).deleted
> -    files = [f for f in ctx.files() if f not in deleted and is_lintable(f)]
> +def runESLint(print_func, files):
> +    """Runs ESLint on the files that have changed.

s/have changed/are passed/
Attachment #8862910 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)

Comment 12

7 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbf8d7206c88
Make the ESLint Mercurial pre-commit hook be run in and pass flake8 validation. r=mossop
https://hg.mozilla.org/integration/autoland/rev/628c847a24e7
Add a git pre-commit hook for running ESLint. r=mossop

Comment 13

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbf8d7206c88
https://hg.mozilla.org/mozilla-central/rev/628c847a24e7
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.