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

chore: pre-commit #8

Closed
wants to merge 4 commits into from
Closed

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Mar 22, 2022

Adding pre-commit. I did not add black yet, though I'd highly recommend it; given the desire to avoid clang-format, I wasn't sure if black would be accepted.

Followup to #4.

I would highly recommend trying out using pre-commit locally. It is really an amazing developer tool. You can see my recent slides on it here.

I didn't turn on testing for it - I'd recommend using pre-commit.ci, as it will auto-fix PRs too, and keep the pins up to date; but if you'd rather GHA, I can add that.

  • docs: spelling fix
  • chore: add pre-commit file & minor touchup
  • style: run pre-commit

@wjakob wjakob force-pushed the master branch 2 times, most recently from 7501401 to 5d92f75 Compare April 5, 2022 12:27
@wjakob
Copy link
Owner

wjakob commented Apr 5, 2022

(Sorry for the big delay -- I still plan to look into this but haven't gotten to it yet)

@henryiii
Copy link
Contributor Author

henryiii commented Apr 5, 2022

Take your time. I can rebase whenever you need me to. Would you be interested in enabling pre-commit.ci on the three example repos in pybind11? I think it was enabled for pybind11 itself, but it's not on the example repos, and that would be very nice to keep up to date and correct user contributions automatically.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Contributor Author

Rebased.

@wjakob
Copy link
Owner

wjakob commented May 23, 2024

Hi Henry,

this PR has been up for a long time, and you even rebased it once to keep track of changes. I appreciate this very much. I also played with pre-commit to see how I might integrate it. Altogether, I totally get why you created this PR and find it a good default for some community projects. (And in general, the guidelines that you provide for other open source projects are a great start).

But what can I say ... I just don't like it :-(. It find it adds too much "process" for what is at the end of the day my hobby project. I want a zero-friction way to concentrate the few minutes I have sometimes to work on this project, and this adds one rather complicated piece of technology that would now lie between me and the github repository. So rather than leaving this PR open for another 2 years, I will close it to reflect the reality, which is that I don't really want pre-commit for this repo. I think that the total amount of time it will save me is less than the amount of time I will need to invest to fully understand it, keep it running, fix flakes, and get my own changes to conform to its wishes.

Wenzel

@wjakob wjakob closed this May 23, 2024
@henryiii
Copy link
Contributor Author

I sadly was about to update this PR, I've found GitHub's "every PR you have open list", and have slowly been working though it. (Actually I had fun writing a GraphQL query that produced the list before finding out it's basically a button in GitHub.)

FYI, this has become quite a bit cleaner and simpler in the last year or two with ruff taking over most of the smaller projects and doing a lot with just a single very fast binary.

You don't have to add checks you don't want to mess with, but having the ability to add arbitrary checks, written in over a dozen languages, including ones you write yourself, is incredibly powerful. It's also something you can run in CI or locally, unlike just adding lots of custom runs of things in CI.

For example, for iris-hep.org's repo, I disallowed adding the wrong file extension when I found people were uploading extensions that were not getting picked up and rendered. I added custom schema files and a check, which swapped the terrible, one a a time errors while building the site (~2-3 mins) with a clean list of every mistake, with a few seconds feedback time. I also even added auto-formatting, which has helped users unfamiliar with yaml. Most users have no idea what pre-commit is, but it runs in CI and even auto fixes some mistakes. We spend much less time reviewing PRs now there.

Another one I've used that is incredibly powerful is auto file regeneration. Using a tool like cog or a custom Python script, you can ensure that a generated file is always regenerated from its source. For example, the Scientific Python Developer's Guide uses it to regenerate the code examples in the pages to be the output of the cookiecutter template it also provides. I've also used it to make sure a requirements.txt file was regenerated from a pdm lock file.

I believe every repo I am a maintainer on (over 40) uses pre-commit, along with basically all my personal ones. Over 63K GitHub repos use it, including CPython itself. Just knowing it is a useful skill. I would treat it like a test suite that runs really quickly, but was designed to check static things and supports checks that change things.

Even if I'm the only committer, it removes many ways to make a mistake - the Schema check has saved me from pushing broken GitHub Actions workflows. Validate-pyproject catches errors in pyprojects - I have downloaded every pyproject.toml file from PyPI, and there are a lot of errors for anything that isn't checked at the 1-5% level.

You don't have to "install" if you don't want to, and just run it directly (I do that most of the time), or even just run it in CI. If you don't push directly and always make PRs, you can even have pre-commit.ci run and auto-push fixes to PRs. It's like having a helpful coworker that always cleans up your work a few seconds after you do a quick and sloppy push. If you don't like formatters, don't add formatters to your pre-commit file. But linters and validators are still really useful.

I'll not make another PR adding pre-commit, then. Just wanted to dump my thoughts; pre-commit is the one tool I use everywhere and rely on more than any other. (It also frustrates me that the author is also the author of Flake8 and hates the Astral team for making Ruff and will never add uv support; I'd love for the setup phase of pre-commit for Python extensions to be powered by uv, as it basically makes all pip installs take 0 seconds.)

I'll keep in mind that you consider nanobind a "hobby project", good to know if people ask which one to use. :P

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

3 participants