pauleveritt / psc

Prototype of new repo for the PyScript Collective.
Other
0 stars 0 forks source link

Concerns about pre-commit hooks #33

Open tildebyte opened 2 years ago

tildebyte commented 2 years ago

I'll say right up front that I have no objection to pre-commit hooks; I'm basically in a position of trying to figure out their finer points.

For context, my day job uses pipelines[^1], including linters etc., on push to branch (e.g. to open a PR from), and on merge to main.

  1. Big one: AFAICT, pre-commit hooks are unavailable for the GitHub web editor/IDE (which has the advantage of being a low barrier to entry for new contributors).
  2. New contributors will have to be instructed to execute pre-commit install (with maybe other args?) after git clone/pip(x) install in order to enable hooks
  3. New contributors won't be made aware of issues until they actually attempt to make a commit.
  4. New contributors are not likely to understand what they're seeing when a hook fails - now it's a remote troubleshooting issue.
  5. Every commit triggers a full local CI run? So, if I'm doing isolated atomic commits in one PR, and I happen to have four or five, and I end up needing to do interactive rebase several times to fix things up before I finally push to open a PR, I'm running pre-commit hooks A LOT.
  6. For some specifically commit-related items, I can see where it would make a lot of sense - DCO, for instance, or Conventional Commits.

Some responses:

  1. vs. having e.g flake8/mypy/etc. running from your editor/IDE on every file save[^2] (or even realtime - flake8 is quite good for this and well fast enough).
  2. Leading a zoom (or whatever) call to walk someone through their first big GitHub workflow pipeline failure is much easier than them sharing their screen and their mentor straining their patience muscles; both can look at exactly the same GitHub outputs at the same time, each on their own system. For that matter, CI on PRs means that anyone can fix the CI issues, whereas pre-commit issues are stuck on the contributor's local system.

[^1]: Generically. Specifically: GH Workflows with custom Actions/GitLab CI Pipelines/ Jenkins jobs (for long-running integration pytests) [^2]: This has the pedagogical advantage of "proximity" - coders can see a mistake right after they make it - this makes it much easier to remember and internalize the fix

pauleveritt commented 2 years ago

These are all very good points. For the record, I don't do pre-commit as a hook. I just run it periodically.

It's quite likely that pre-commit/mypy/typeguard/nox are just way, way too much to ask. It's more probable that the collaborators perform more of the work to get someone's code to pass all the stuff.

But we might be kidding ourselves -- the desires to (a) ensure code quality to manage our burden and (b) give a positive example of learnable code -- just might erect too high a barrier.

We can start removing stuff from the noxfile and GHA until we have the right balance.

tildebyte commented 2 years ago

That sounds good. Let's see how terrible it is to start with, and then adjust 😁

OTOH

...{these} might erect too high a barrier (a) ensure code quality to manage our burden (b) give a positive example of learnable code

I feel like this should be part of the mission here, at the very least in terms of helping people learn to use PyScript.

My suspicion is that PyScript is going to attract new developers with NO experience in exactly the same way that JavaScript attracted new developers when it was new (lord knows JavaScript isn't a "low barrier" to ANYTHING, and was even worse at the beginning) - I feel like this repo and website might end up being the "tip of the spear", and in that context, our code quality and best-practices stance could end up being very important...

pauleveritt commented 2 years ago

Let's get specific. To achieve the balance, what code quality tools changes would you like us to make?