1360595 - Add a git pre-commit hook to run ESLint

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

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 1

[Mark Banner (:standard8)]

Attachment: Bug 1360595 - Make the ESLint Mercurial pre-commit hook be run in and pass flake8 validation.

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

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1360595 - Add a git pre-commit hook for running ESLint.

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

Comment 3

[Dave Townsend [:mossop]]

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

Comment 4

[Dave Townsend [:mossop]]

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.

Comment 5

[Mark Banner (:standard8)]

(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?

Comment 6

[Dave Townsend [:mossop]]

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.

Comment 7

[Mark Banner (:standard8)]

(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 8

[Mark Banner (:standard8)]

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

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

Comment 9

[Mark Banner (:standard8)]

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

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

Comment 10

[Dave Townsend [:mossop]]

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/

Comment 11

[Mark Banner (:standard8)]

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

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/bbf8d7206c88
https://hg.mozilla.org/mozilla-central/rev/628c847a24e7