1230300 - Add a hg extension to reject changesets that don't pass eslint

Status: RESOLVED FIXED

(Testing :: General, defect)

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1230300: Fix mach eslint to pass command arguments through to eslint. r?gps

Bug 1230300: Fix mach eslint to pass command arguments through to eslint. r?gps

Currently mach treats the first argument to eslint as the path and moves it to
the end of the arguments but this breaks usage like "mach eslint -f json browser".

It used to be necessary to change to the directory you wanted to lint but now
the .eslintignore is at the top level we just run from the top level. This means
the path argument doesn't need to be special anymore.

Comment 2

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1230300: Add a hg hook that rejects changesets that fail to pass eslint. r?gps

Bug 1230300: Add a hg hook that rejects changesets that fail to pass eslint. r?gps

This grabs the list of changed and added files that match the set we expect to
be able to lint and runs them through mach eslint. Warnings are ignored partly
because otherwise we see warnings for files that are ignored in .eslintignore.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8695452 [details]
MozReview Request: Bug 1230300: Fix mach eslint to pass command arguments through to eslint. r?gps

https://reviewboard.mozilla.org/r/27107/#review24551

::: python/mach_commands.py:176
(Diff revision 1)
> -    def eslint(self, setup, path, ext=None, binary=None, args=[]):
> +    def eslint(self, setup, ext=None, binary=None, args=[]):

While you're changing this line, please change `args=[]` to `args=None`. The way Python evaluates default function arguments is "special." Quick version: never use complex types like lists and dicts or bad things can happen.

::: python/mach_commands.py:197
(Diff revision 1)
> -        self.log(logging.INFO, 'eslint', {'binary': binary, 'path': path},
> -            'Running {binary} in {path}')
> +        if len(args) == 0:
> +            args = ["."]

empty lists and dicts evaluate as False. So testing for empty list is typically `if not args:`. These 2 lines can be reduced to `args = args or ['.']`.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8695453 [details]
MozReview Request: Bug 1230300: Add a hg hook that rejects changesets that fail to pass eslint. r?gps

https://reviewboard.mozilla.org/r/27109/#review24561

::: python/eslint_hook.py:14
(Diff revision 1)
> +    except:

Bareword except is evil in Python because it swallows exceptions like KeyboardInterrupt and SystemExit, which are sent after certain signals. Use `except Exception:` instead.

::: python/eslint_hook.py:15
(Diff revision 1)
> +        return False

So, precommit hooks that fail cause the commit to fail. I think that aborting the commit is wrong in almost all circumstances. Here's why.

It is common to think of version control tools as filesystems or file editors. In this analogy, "commit" can be equated with "save file." Having precommit hooks that prevent you from actually committing are essentially preventing "save file."

Extending this analogy to the area of static analysis, imagine a document editor that upon request to save a file said "refusing to save: 'colour' is not a word." I'm guessing you would be extremely frustrated (and it isn't [just] because our cultures have different spellings for "color").

Imagine you were hacking on some quick changes to debug something. You just want to create throwaway, low quality commits to test things. With hooks that enforce things like style conformance, these quick experiments become harder and the hook's strictness gets in the way.

IMO the proper action for commit-time hooks is to print warnings. If the user sees a warning, they can fix it real quick and `hg amend` (or whatever) to create a new version of the commit if desired. If people ignore the warnings, that's what review-push-time checks and MozReview bots are for.

Please have this hook always succeed.

::: python/eslint_hook.py:22
(Diff revision 1)
> +    files = check_output(["hg", "status", "-man", "--change", node]).split("\n")

So, Python processes have a non-trivial amount of startup overhead. It's in the several dozen milliseconds range. This is a reason a lot of `hg` commands aren't instantaneous even though the stuff actually happening inside Mercurial is pretty much instantaneous.

Anyway, the way this hook is currently configured will result in:

1. (depending on how the hook is registered) `hg` spawning a new Python process to run this hook file
2. A new Python process being invoked to run `hg status`.

On top of that, various internal state that the original `hg` process has cached has to be recomputed for the new `hg status` command. Overall, this almost certainly adds up to >100ms of delay.

The way I like writing hooks is as a minimal Mercurial extension. See https://hg.mozilla.org/hgcustom/version-control-tools/file/ab7ef9952a97/hgext/checkpystyle/__init__.py for a Python version of this one that does most of what you want.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #4)
> ::: python/eslint_hook.py:22
> (Diff revision 1)
> > +    files = check_output(["hg", "status", "-man", "--change", node]).split("\n")
> 
> So, Python processes have a non-trivial amount of startup overhead. It's in
> the several dozen milliseconds range. This is a reason a lot of `hg`
> commands aren't instantaneous even though the stuff actually happening
> inside Mercurial is pretty much instantaneous.
> 
> Anyway, the way this hook is currently configured will result in:
> 
> 1. (depending on how the hook is registered) `hg` spawning a new Python
> process to run this hook file
> 2. A new Python process being invoked to run `hg status`.
> 
> On top of that, various internal state that the original `hg` process has
> cached has to be recomputed for the new `hg status` command. Overall, this
> almost certainly adds up to >100ms of delay.
> 
> The way I like writing hooks is as a minimal Mercurial extension. See
> https://hg.mozilla.org/hgcustom/version-control-tools/file/ab7ef9952a97/
> hgext/checkpystyle/__init__.py for a Python version of this one that does
> most of what you want.

So I would agree, and I even tried to do something similar but the difficulty of registration is a problem. With an external hook it is as simple as adding a single line to your .hgrc to register it. Anything else and you have to start installing the hook or extensions somewhere in the python path.

Comment 6

[Gregory Szorc [:gps]]

(In reply to Dave Townsend [:mossop] from comment #5)
> So I would agree, and I even tried to do something similar but the
> difficulty of registration is a problem. With an external hook it is as
> simple as adding a single line to your .hgrc to register it. Anything else
> and you have to start installing the hook or extensions somewhere in the
> python path.

(Please reply to MozReview comments on MozReview, as comments are not mirrored in both directions. You get proper threading when you comment on MozReview. Bug 1225972 tracks adding some UI to Bugzilla to discourage replies in Bugzilla.)

An extension can also be registered in a single line, just like a hook:

  [extensions]
  mozeslint = ~/src/firefox/python/eslint_hook.py

The path to the extension does not need to be in the Python path.

A major benefit of doing things as extensions is you can add much more powerful functionality later without having to require users to muck around with hgrc files to update references to then-defunct hooks.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d4a2076b0592

Comment 8

[Dave Townsend [:mossop]]

Landed the first part of this since it is useful anyway

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d4a2076b0592

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8695453 [details]
MozReview Request: Bug 1230300: Add a hg hook that rejects changesets that fail to pass eslint. r?gps

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

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8695453 [details]
MozReview Request: Bug 1230300: Add a hg hook that rejects changesets that fail to pass eslint. r?gps

https://reviewboard.mozilla.org/r/27109/#review25003

This looks good. I'm not too keen on python/ being the home for Mercurial extensions. Can you check this in under tools/mercurial/eslintvalidate.py (feel free to change the basename of the extension)?

Also, we have a weird tension between version-control-tools in mozilla-central right now and I could make the case for adding this file to version-control-tools (especially since we have test infrastructure there). I intend to eventually merge version-control-tools into mozilla-central. But I can't do that until we change some of the tree rules and automation for mozilla-central. For now, version-control-tools is effectively a non-vendored sub-directory of mozilla-central. Either way, you probably want to follow this up with an addition to `mach mercurial-setup` to install this new extension.

::: python/eslint_hook.py:1
(Diff revision 2)
> +import os, sys

This file needs a GPL header since it calls into Mercurial APIs.

 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2d95a1f997e8

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d95a1f997e8