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

add template for pipenv, update install routine #20

Closed
wants to merge 7 commits into from

Conversation

LeoIV
Copy link
Contributor

@LeoIV LeoIV commented Apr 25, 2019

There is an issue with autohooks being installed in pipenv environments. The hook is installed correctly but it cannot be run because it starts a python3 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:

@LeoIV LeoIV requested a review from bjoernricks as a code owner April 25, 2019 14:29
@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

Merging #20 into master will decrease coverage by 0.19%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
autohooks/install.py 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef13ed6...7b6500b. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 25, 2019

Codecov Report

Merging #20 into master will decrease coverage by 13.1%.
The diff coverage is 8.33%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ
autohooks/install.py 0% <0%> (-64.52%) ⬇️
autohooks/config.py 92.68% <33.33%> (-4.69%) ⬇️
autohooks/utils.py 26.53% <0%> (-55.83%) ⬇️
autohooks/api/git.py 0% <0%> (ø) ⬆️
autohooks/precommit/run.py 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54d0bb6...54edc6f. Read the comment docs.

Copy link
Member

@bjoernricks bjoernricks left a 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.

setup_dir_path = get_autohooks_directory_path()
return setup_dir_path / 'precommit' / 'template'
# check if in pipenv
ret_code = os.system("pipenv graph")
Copy link
Member

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

@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 25, 2019

I didn't read the README.md careful enough. I think it would be useful for many applications if the hooks are installed and enabled automatically with the project checkout.

I don't see how this can be done with a command line switch as this requires the user to set this option explicitly.

@bjoernricks
Copy link
Member

I really would like to support this use case! Maybe some we could use a setting in the pyproject.toml file instead?

@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 25, 2019 via email

add explanation ot README.md
update black version in Pipfile
@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 26, 2019

@bjoernricks I added the option which can be enabled in the pyproject.toml (details are in the README.md)

Copy link
Member

@bjoernricks bjoernricks left a 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)

@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 26, 2019

Done

Copy link
Member

@bjoernricks bjoernricks left a 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.

@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 26, 2019

Nah, you're right. The hook is installed just like the other but is run automatically.

Copy link
Member

@bjoernricks bjoernricks left a 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.

README.md Outdated Show resolved Hide resolved
autohooks/config.py Outdated Show resolved Hide resolved
autohooks/install.py Outdated Show resolved Hide resolved
@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 27, 2019

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 README.md?

@bjoernricks
Copy link
Member

By the way: I wrote a first version of an autopep8 plugin. How do you think about adding a link to it to the README.md?

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.

@bjoernricks
Copy link
Member

I implemented your change requests. Please let me know your decision regarding the naming and the default behavior.

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: mode = "pipenv".

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.

@LeoIV
Copy link
Contributor Author

LeoIV commented Apr 29, 2019

That sound reasonable. How would you like to proceed with this PR then?

@bjoernricks
Copy link
Member

You could rebase your commit on top of my changes.

@bjoernricks
Copy link
Member

Just added a PR for the mode config api in #24

@LeoIV
Copy link
Contributor Author

LeoIV commented May 1, 2019

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.

@LeoIV LeoIV closed this May 1, 2019
@bjoernricks
Copy link
Member

Thanks for your PR and your willingness to support the feature :-)

Really would love to get you proposed feature include. Just a matter of time and help.

I think the changes in this PR are contained in your PR, so I will close this.

It's still missing the actual installation of the pre-commit hook from the template.

@bjoernricks
Copy link
Member

@LeoIV just wanted to let you know i've finished my work on #24 and it will get merged into master next week. With #24 it will possible to auto activate the pipenv env when running the git hook.

Btw. would love to get a PR to mention your pep8 plugin at the README!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants