Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre-commit Can't Find Black #2299

Closed
HassanAbouelela opened this issue Jun 2, 2021 · 4 comments
Closed

Pre-commit Can't Find Black #2299

HassanAbouelela opened this issue Jun 2, 2021 · 4 comments
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases

Comments

@HassanAbouelela
Copy link
Contributor

Describe the bug

When committing files, pre-commit tries to call the system-wide black instead of the one installed in the virtual env. This can either mean the committed files are formatted with an incorrect version of black, or if black is not installed on a system-wide level, the commit will fail. I successfully reproduced this on windows and Linux.

(This is for the local development hooks, not another project using black as a hook.)

Note that this behavior is unique to committing, and can't be replicated using pre-commit run.

To Reproduce

  1. Run pre-commit install
  2. Make a change to a python file
  3. Make sure black isn't installed system-wide.
  4. Try to commit your changes.
  5. Pre-commit will exit with an error about not finding black.

Expected behavior

The pre-commit will find the correct black version, and run it on the staged files.

Environment (please complete the following information):

  • Version: main
  • OS and Python version: Windows/3.9.4, linux/3.9.5
  • Pre-commit: 2.12.1

This info is mostly irrelevant, and this bug can be reproduced under any environment.

Does this bug also happen on main?

Yes

Additional context

This has been discussed on discord here. It's a pretty common problem with local pre-commit hooks.

I've run into this before, and there are a few ways to approach this problem. First, a little background about the problem:
pre-commit doesn't activate your venv when running the hooks. The reason is not really important, but the library maintainers have explicitly stated they aren't interested in doing that. That leaves end users to deal with these problems themselves.

Onto the fixes:

  • There are a few fixes that can make this work. They mostly revolve around activating the environment ourselves. This can be achieved through direct manipulation of the pre-commit file. The downsides with this is this fix can not be checked into the repo, and must be done manually by contributors trying to use the hooks.
  • Another fix that activates the virtualenv is explicitly running the command using pipenv. To do this, the entry point should be set to pipenv run black instead of just black. The downside to this is the pre-commit hook doesn't work if you're not using pipenv (admittedly that seems like a pretty niche scenario).
  • A different idea for a fix would be using a remote black version. This will alleviate all the problems managing environments, and offloads the problem to pre-commit. The main disadvantage will be the pre-commit hook won't be running the same behavior as the code you're writing. That is probably not an issue except in very limited scenarios.

Finally, a few things that I tried and didn't work: changing language to python, and trying to run black as a module using python -m src/black as an entrypoint.

@felix-hilden
Copy link
Collaborator

Hi! Yeah, as discussed this is a problem for me too. But I don't think using a remote Black version is the solution. You said it wouldn't be a problem except in very limited scenarios, but as Black is still in beta and the style is changing, I would consider not being able to apply your own changes a huge downside.

I wouldn't mind using pipenv run black, because it's already the chosen virtual environment. Requiring contributors to use it shouldn't be too much to ask.

@ichard26
Copy link
Collaborator

ichard26 commented Jul 3, 2021

I wouldn't mind using pipenv run black, because it's already the chosen virtual environment. Requiring contributors to use it shouldn't be too much to ask.

I agree. 1 sounds like it would make it harder to contribute to Black which is not what we are trying to do here :) and 2 would be a problem because the entire Black codebase is formatted as part of the test suite (not sure why but don't fix what's not broken I guess?) so it must use the current style and not stable's style.

@ichard26 ichard26 added the C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases label Jul 3, 2021
@HassanAbouelela
Copy link
Contributor Author

This should be a relatively easy change, but one problem we'll run into are the actions. The lint action installs everything using pip, then runs the pre-commit, so pipenv wouldn't be available in that case. Should I amend the action?

@hauntsaninja
Copy link
Collaborator

Fixed in #3114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases
Projects
None yet
Development

No branches or pull requests

4 participants