-
Notifications
You must be signed in to change notification settings - Fork 22
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
add template for pipenv, update install routine #20
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
=========================================
- Coverage 18.15% 17.96% -0.2%
=========================================
Files 10 10
Lines 369 373 +4
=========================================
Hits 67 67
- Misses 302 306 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
===========================================
- Coverage 31.09% 17.98% -13.11%
===========================================
Files 10 10
Lines 373 378 +5
===========================================
- Hits 116 68 -48
- Misses 257 310 +53
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about your use case.
The intention of autohooks is to run the hooks only if the user is within a pipenv environment. With this approach the user can decide to use autohooks by attaching/detaching to the environment.
But I am open for adding a command line switch to install your new template and running autohooks via pipenv run.
autohooks/install.py
Outdated
setup_dir_path = get_autohooks_directory_path() | ||
return setup_dir_path / 'precommit' / 'template' | ||
# check if in pipenv | ||
ret_code = os.system("pipenv graph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please always use subprocess instead of os.system
I didn't read the I don't see how this can be done with a command line switch as this requires the user to set this option explicitly. |
I really would like to support this use case! Maybe some we could use a setting in the pyproject.toml file instead? |
Perfect, I will look into this soon 🙂
… On 25. Apr 2019, at 17:12, Björn Ricks ***@***.***> wrote:
I really would like to support this use case! Maybe some we could use a setting in the pyproject.toml file instead?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
add explanation ot README.md update black version in Pipfile
@bjoernricks I added the option which can be enabled in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the changes of the Pipfile and Pipfile.lock from this commit? black 19.3b0 is broken for python 3.5 (psf/black#759)
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if i get this right. From your code it looks like you are installing a git hook that runs autohooks independent of being inside of the active pipenv environment.
But your addition to the README (and the new config value) is talking about auto installation. Auto installation of the git hook should work already when installing autohooks as a dependency from pypi. But maybe this broke with pip 19.1 too.
Nah, you're right. The hook is installed just like the other but is run automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not satisfied with auto-run as name yet. Doesn't reflect the real purpose. I am also thinking about adding a command line switch for the activate command that sets this variable. Additionally the new behavior could be the new default maybe. I guess most users are expecting this.
I implemented your change requests. Please let me know your decision regarding the naming and the default behavior. By the way: I wrote a first version of an autopep8 plugin. How do you think about adding a link to it to the |
Nice. Would be fine. Just double check your sources for old black references. Personally I am thinking about more generic plugins for linting and formatting where theactual formatter and linter can be set via the config. |
I've thought a lot about this feature at the weekend. I would like to use a mode setting instead of a single boolean setting to be more extendable for the future: Additionally I would like to convert the template hook file into a real template instead of providing two files. I've already laid out the groundwork for this change yesterday. |
That sound reasonable. How would you like to proceed with this PR then? |
You could rebase your commit on top of my changes. |
Just added a PR for the mode config api in #24 |
Thanks for your PR and your willingness to support the feature :-) I think the changes in this PR are contained in your PR, so I will close this. |
Really would love to get you proposed feature include. Just a matter of time and help.
It's still missing the actual installation of the pre-commit hook from the template. |
There is an issue with
autohooks
being installed inpipenv
environments. The hook is installed correctly but it cannot be run because it starts apython3
environment outside of the pipenv. However,autohooks
cannot be imported in this case as it is only known in the virtual environment.This PR checks if a Pipenv is present in the current project. If this is the case, it uses a different template that starts the hook with
pipenv run python3
.Checklist: