(Feature Request) Mandatory hooks / less graceful ImportError failure

[alex-thewsey-ibm]

Hi team & thanks for a great library!

Our team uses an app development workflow based on pyenv and poetry - with autohooks and its plugins installed locally to the project+virtual environment.

Our devs are often not actually in the virtual environment shell when doing stuff: Instead using commands like poetry run to trigger scripts and poetry add to update dependencies.

Unfortunately this often includes not being in the env when running git commit, and autohooks is currently pretty lax about this per the example pre-commit hook below:

#!/usr/bin/env python3

import sys

try:
    from autohooks.precommit import run
    sys.exit(run())
except ImportError:
    print('autohooks not installed. Ignoring pre-commit hook.')
    sys.exit(0)

I would love to see the ability (maybe via configuration if it's not for everybody?) to fail more forcefully in this case: For us it's much more likely that a forgetful developer is committing from the wrong environment than that there's an installation issue we want to tolerate!

[bjoernricks]

Hi,

thanks for using autohooks :-)

If I understand you correctly I already did work on this problem in #24
With this PR it will be possible to run autohooks within pipenv automatically. Not sure if it will fix your specific issue (because you aren't using pipenv I suppose) but I am sure it can be extended to be more flexible in this kind. Sadly I hadn't time in the past weeks to finish the PR.

[alex-thewsey-ibm]

Ah it looks like your approach was a bit different to my idea: modify the shebang to make it work from outside, rather than modify the error to make it fail... Certainly cooler if we can get it working!

I see a couple of potential roadblocks/opportunities though:

1. Cross-platform limitations of shebang
From this and related discussions, I think the PR might die on a non-trivial selection of Linux flavours that interpret the complex #!/usr/bin/env pipenv run python3 shebang as "find me an executable called 'pipenv run python' (one filename)". Perhaps this cool polyglot shell/python script could be a basis for doing the same thing with a straight shell shebang and more portability?

2. python3?
IIRC pipenv (and poetry too) virtualise python regardless of whether the env is using 2 or 3, so it seems like maybe pipenv run python would be less magical/fragile... Unless it's possible/practical to have a pipenv containing both python2 (the app under development) and python3 (on which autohooks is installed)?

3. Poetry is easy, but the rest..?
poetry run python should be a pretty copy&paste stand-in for pipenv run python, which is great (I arrived at poetry as a pipenv refugee myself after giving up with the long locking times!) ...but I'm a bit worried about the potential for getting swamped in supporting different virtual env tools. Would suggest a "manual" config option where users can specify custom python invokations (which doesn't seem to be what Mode.MANUAL does in #24 right? Maybe I misunderstood) - perhaps using shell script (per point 1 above) to invoke the hook script with whatever is supplied.

4. There's still a use case for fail on ImportError
It's a great efficiency for the developer workflow if we can get virtual-env aware hook execution; but at the end of the day we're also making the hook execution more magical. Developers can always add --no-verify if they really need to push something through, but I don't think our team would be that unusual in wanting pre-commit to fail, rather than pass, when a hook is installed but misconfigured due to Python environments?

And a quick Q: Sorry for my ignorance of toml & Enum: Is the PR expecting an entry like mode = "pipenv" or mode = 1?

[bjoernricks]

1. Cross-platform limitations of shebang

Thanks for the pointer. Wasn't aware of this issue. But IMHO it can be fixed with the -S parameter of /usr/bin/env

/usr/bin/env -S pipenv shell python3

works for me.

[bjoernricks]

2. python3?

I've dropped my usage of python 2 completely therefore python is python 3 for me always. Of course that's not true for all people yet and pipenv shell python3 should be used instead.

[bjoernricks]

Poetry is easy, but the rest..?

Personally I would add a poetry mode only and only add the flexibility for other tools on demand.
Adding flexibility without a real demand creates a lot of headaches and maintenance costs.

Mode.MANUAL currently means you have to put autohooks into your python path manually e.g. by activating your environment. I didn't found a better name for that mode yet ;-)

Developers can always add --no-verify if they really need to push something through,

When I started to write autohooks I wasn't aware of this git switch and didn't wanted to always fail if the dependencies aren't installed. But now I think this approach wasn't a good decision and shouldn't be the default anymore. Therefore I've started to work on #24

There's still a use case for fail on ImportError

Yes you are right. I really think not failing if the dependencies aren't available should only be an option anymore and not the default. This needs to be changed in the next major release. But to fix this situation I wanted to get a sane way to run autohooks without having to care about activating an environment before.

[bjoernricks]

@alex-thewsey-ibm I've finished my work on #24 and it will get merged into master next week. Afterwards adding support for poetry would be very easy. I've changed the installed hook script to fail if autohooks couldn't be imported. It will need overriding the existing pre-commit hook script by running autohooks activate --force.

[bjoernricks]

@alex-thewsey-ibm with #24 #28 and #29 this issue seems to be fixed for me. Please re-open if something is still missing.