Switch to use pre-commit config YAML file

pschanely / CrossHair

An analysis tool for Python that blurs the line between testing and type systems.

Other

1.04k stars 49 forks source link

Switch to use pre-commit config YAML file #181

Closed tekktrik closed 2 years ago

tekktrik commented 2 years ago

Resolves #121 by swapping precommit.py for the standard .pre-commit-config.yaml file. I tried my best to replicate the existing actions performed by the old file, and also tried to fix any errors that popped out. There are a few remaining from some of the hooks, so please feel free to push any fixes for them directly to this PR.

Setting this as a draft for now, if everything looks good I can undraft it.

Let me know if you have any questions, or if there's anything I can explain!

tekktrik commented 2 years ago

Whoops, I forgot to add pre-commit as a dependency!

tekktrik commented 2 years ago

I pinned pre-commit with compatibility release notation to 2.20 (latest), but it could almost certainly be relaxed if there are any use cases requiring it.

tekktrik commented 2 years ago

It looks like even pre-commit run --all-files alone wasn't running the pytest hook, so I patched that by adding --hook-stage manual to trigger it as well.

tekktrik commented 2 years ago

Here are the failures that I see so far:

There are two errors from pydocstyle for missing argument descriptions. If you want to add those directly to this PR, it's open for changes to maintainers.
It also looks like there's a doctest error but I'm having a harder time understanding where the error is there.

tekktrik commented 2 years ago

I fixed the doctest issue with that fix as well, but I guess one of tests in the pytest suite requires mypy and so that one failed. Not sure if that means you want to keep mypy as a dev dependency, or whether the test should be changed. Let me know!

pschanely commented 2 years ago

I fixed the doctest issue with that fix as well, but I guess one of tests in the pytest suite requires mypy and so that one failed. Not sure if that means you want to keep mypy as a dev dependency, or whether the test should be changed. Let me know!

You are fast! Yeah, I forgot about that mypy test: let's just add that mypy dev dependency back in for now. Thank you!

pschanely commented 2 years ago

There are two errors from pydocstyle for missing argument descriptions. If you want to add those directly to this PR, it's open for changes to maintainers.

It's a little odd that this exclusion in setup.cfg doesn't skip it; I think it has to do with pre-commit passing filenames directly. Anyway! Yup, I'll just fix up the two errors instead of thinking about it.

tekktrik commented 2 years ago

I should be able to mimic that exclusion in the hook, I think, if you want to go that route as well.

tekktrik commented 2 years ago

Yup, tested locally, I can exlude it by passing --match='(?!datetimelib).*(?<!_test).py$' as an arg to the hook.

pschanely commented 2 years ago

Yup, tested locally, I can exlude it by passing --match='(?!datetimelib).*(?<!_test).py$' as an arg to the hook.

Thanks for investigating! I just pushed something to resolve the two errors in datetimelib.py - a reasonable thing to do anyway.

pschanely commented 2 years ago

Checks look good, so I've merged. Thank you -- I'm pretty excited about the other hooks we can trivially add now!

I don't think I should have to, but adding the "hacktoberfest-accepted" label to make extra sure it counts :smile:

tekktrik commented 2 years ago

Thanks so much! I always love working on repo infrastructure. Feel free to reach out if anything like that comes up!