jendrikseipp / vulture

Find dead Python code
MIT License
3.41k stars 150 forks source link

add a pre-commit hook config file #244

Closed neutrinoceros closed 3 years ago

neutrinoceros commented 3 years ago

Description

Adding this file to the repo makes vulture usable as a hook for pre-commit. As such, it can be easily integrated to a shared development workflow and to CI (in particular within pre-commit.ci)

Related Issue

none

Checklist:

neutrinoceros commented 3 years ago

I was able to test this hook locally and did not encounter any obstacle.

Screen Shot 2021-01-13 at 19 15 09

Important note: in my addition to README.md, I'm pointing to what I think is the next logical release for vulture (2.2), but this is only relevant if said release is done immediately following this merge. If there's a better way to accommodate your workflow please tell me !

jendrikseipp commented 3 years ago

Thanks! The code looks good, I modified it slightly though. However, I'm not sure how useful the pre-commit integration will be, because it will test modified files and maybe even only one file at a time. Can you test this and confirm these two things, please?

Is there a way to always check all Python files tracked by the Git repo?

Also, can you check that the pre-commit invocation really uses the Vulture configuration from the pyproject.toml file?

neutrinoceros commented 3 years ago

it will test modified files and maybe even only one file at a time. Can you test this and confirm these two things, please?

I confirm that it checks only modified files (unless run with --all-files flag as in my screenshot above, which is also what pre-commit-ci does underneath), but it catches them all: not just one by one.

Is there a way to always check all Python files tracked by the Git repo?

I'm pretty sure there is, but I need to inquire this further. I'll get back at you !

Also, can you check that the pre-commit invocation really uses the Vulture configuration from the pyproject.toml file?

Yup. Tested on a large (and old) repo, and I confirm that my configuration is correctly used. This is what I used:

# pyproject.toml
[tool.vulture]
min_confidence = 100

with this conf

Screen Shot 2021-01-13 at 20 27 32

and without it

Screen Shot 2021-01-13 at 20 27 55

(both screen shots are cut off, you get the point)

neutrinoceros commented 3 years ago

Is there a way to always check all Python files tracked by the Git repo?

So I haven't worked my head around it yet but as a note the hook for https://github.com/asottile/dead does it (asottile is also the author of pre-commit), so it's definitely possible !

Writing this I am also realising I may have misinterpreted your question about testing files one by one... of course you meant to check wether vulture is still able to connect the dots if something is unused in the file where it's defined but imported elsewhere. I think that indeed it only looks at the subset of modified files so one gets pretty garbage reports in that state (which I didn't realise because everything looks good when the min confidence is set to 100%).

neutrinoceros commented 3 years ago

Here is the mechanism Anthony used to systematically run dead against all files: https://github.com/asottile/dead/blob/b3ca1d3bed59300d4bd7c61b76273368b569cddf/dead.py#L212

Replicating this feature in vulture would obviously require a deeper patch. Dead has the luxury of assuming it's run in a git repo, but vulture doesn't. The cleanest option that wouldn't break backward compatibility I'm seeing is adding an argument to vulture (e.g. --git, or something clearer ?) which could be passed instead of a file list. What do you think ?

pquentin commented 3 years ago

The cleanest option that wouldn't break backward compatibility I'm seeing is adding an argument to vulture (e.g. --git, or something clearer ?) which could be passed instead of a file list. What do you think ?

I'm only a vulture user migrating to pre-commit, but that's one honking great idea!

jendrikseipp commented 3 years ago

I think another option is to use pass_filenames: false for pre-commit (https://pre-commit.com/#hooks-pass_filenames) and define the filenames in Vulture's pyproject.toml file. Could you try if that works and adapt the PR accordingly if it does, please? Probably, we also need to set require_serial: true since otherwise I guess multiple Vulture instances might run.

neutrinoceros commented 3 years ago

define the filenames in Vulture's pyproject.toml file.

I don't know how one would do this. Maybe I'm just confused. Could you elaborate ?

The rest (pass_filenames and require_serial) look absolutely necessary indeed. I overlooked the serial/parallel aspect, thanks !

jendrikseipp commented 3 years ago

define the filenames in Vulture's pyproject.toml file.

I don't know how one would do this. Maybe I'm just confused. Could you elaborate ?

I mean that we should add something along the following lines to the README:

To run Vulture before each commit, add the following pre-commit hook to name of yaml file and specify all files that should be checked in the pyproject.toml file under the paths key.

codecov-io commented 3 years ago

Codecov Report

Merging #244 (594d6a2) into master (fdac4e9) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #244   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files          17       17           
  Lines         631      631           
=======================================
  Hits          627      627           
  Misses          4        4           

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 fdac4e9...594d6a2. Read the comment docs.

neutrinoceros commented 3 years ago

Thanks (and done) ! I can also confirm after testing this setup locally that it works like a charm ! I see you just released vulture 2.2 so I'm bumping the suggested version number to use to 2.3 :)

jendrikseipp commented 3 years ago

Thanks!

neutrinoceros commented 3 years ago

Thank you for merging ! can you please bump Vulture's version to 2.3 ?

jendrikseipp commented 3 years ago

Done :-)