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-hooks.yaml #5157

Closed
wants to merge 4 commits into from
Closed

Conversation

maximevast
Copy link

Hi :)

Sorry I'm not compliant with your contributing guidelines but this pr is not about the project itself, it's about being able to use it automatically with pre-commit.
I was looking for a way to run my tests before every commit and was surprised to discover that you use pre-commit for your own development workflow but do not provide a pre-commit-hooks.yaml

So here's one, I'm using it from my fork and it does the job.

I hope you'll accept this little change.

Regards

@blueyed
Copy link
Contributor

blueyed commented Apr 23, 2019

Thanks for the PR, but we have ./.pre-commit-config.yaml already.
Haven't looked at the diff, but you might want to revisit this one then.. :)

@blueyed
Copy link
Contributor

blueyed commented Apr 23, 2019

Oh, this might be about something else? https://github.com/pre-commit/pre-commit-hooks

@codecov
Copy link

codecov bot commented Apr 23, 2019

Codecov Report

Merging #5157 into master will increase coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5157      +/-   ##
==========================================
+ Coverage   95.79%   96.06%   +0.27%     
==========================================
  Files         114      114              
  Lines       25825    25825              
  Branches     2550     2550              
==========================================
+ Hits        24740    24810      +70     
+ Misses        757      705      -52     
+ Partials      328      310      -18
Impacted Files Coverage Δ
src/_pytest/fixtures.py 97.93% <0%> (+0.27%) ⬆️
testing/test_assertrewrite.py 84.04% <0%> (+0.29%) ⬆️
testing/test_capture.py 99.24% <0%> (+0.3%) ⬆️
testing/test_config.py 99.49% <0%> (+0.33%) ⬆️
testing/test_pdb.py 99.19% <0%> (+0.4%) ⬆️
src/_pytest/pytester.py 91.1% <0%> (+0.43%) ⬆️
src/_pytest/assertion/rewrite.py 95.47% <0%> (+0.48%) ⬆️
testing/python/fixtures.py 99.08% <0%> (+0.57%) ⬆️
src/_pytest/junitxml.py 95.2% <0%> (+0.63%) ⬆️
testing/test_parseopt.py 97.97% <0%> (+0.8%) ⬆️
... and 9 more

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 34bc594...e6620d3. Read the comment docs.

@asottile
Copy link
Member

👋 first thanks for the contribution!

I think there's a few reasons this won't work for the general case and often is better suited for a repo: local hook or in tox.in / etc. instead

First and foremost is the technical limitation. since pre-commit creates isolated environments for each hook, they (usually) won't have access to your project and its dependencies (meaning this config would only really work out-of-the-box for a project which had no dependencies, no test dependencies, and was using a non-src layout). Granted a consumer can ~kinda fix that by twiddling additional_dependencies but that's more of a square-peg-round-hole situation.

The second, and perhaps the more important (?) is the ergonomics of such a hook. While I (and others) have been working a bit on improving pytest startup / runtime, it's still a pretty slow task to execute a full test suite. Even if startup time were near zero, running a full test suite on every commit in the critical path adds up quickly (and will likely be the slowest part of that commit) -- if I find a hook is slower than others I tend to disable it, and others will outright disable the whole tool!

That said, there still are situations where I've reached for pytest as a hook in pre-commit, though it wasn't necessarily pretty to make it happen:

https://github.com/pre-commit/pygrep-hooks/blob/06d668d27d7b2db128838f83d31c6ab2bd852910/.pre-commit-config.yaml#L46-L52

@blueyed
Copy link
Contributor

blueyed commented Apr 23, 2019

Ah, now I see what it is about.. :)

Might make sense with something like pytest-testmon (https://github.com/tarpas/pytest-testmon), diff-cover (https://github.com/Bachmann1234/diff-cover) or something similar only really.

But even if it only takes a few seconds I would still prefer to not have it on by default.

@maximevast
Copy link
Author

@asottile, @blueyed Thanks for having considered my pr!
Your explanations make perfect sense, I just thought that enabling pytest as a pre-commit repo would only give more freedom to the devs using both libs.
Anyway, I would totally understand if you'd rather keep it that way and I can perfectly accommodate with a local repo.

Best

@RonnyPfannschmidt
Copy link
Member

i believe there is still some structural merit to those, but i cant quite put it into the right words yet

when pytest no longer uses pkg_resources, it may be sensible to run a core set of unit-tests on commit for example to prevent blunters - there is various small projects where this is entirely sensible

@nicoddemus
Copy link
Member

Thanks @MathersMax for the contribution and understanding, and everyone else for the excellent discussion. 👍

@nicoddemus nicoddemus closed this Apr 24, 2019
@asottile asottile removed their assignment Feb 9, 2020
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

5 participants