search

sandialabs / pyttb

Python Tensor Toolbox
https://pyttb.readthedocs.io
BSD 2-Clause "Simplified" License
26 stars 13 forks source link

Include pre-commit in dev requirements? #280

Closed dmdunla closed 1 year ago

dmdunla commented 1 year ago

A discussion arose in #276 regarding pre-commit, which is currently supported but optional for pyttb developers.

Should we consider making this a requirement? What are the pros and cons of making it a requirement? Can we enforce it as a dev requirement?

ntjohnson1 commented 1 year ago

We can add it as a dev dependency which forces it to be pip installed. But in order for it to run every commit it has to place githooks into the specific clone, so we can't force them to actually use it.

Nbstripout is the only thing that's really relevant to the discussion, and adding lots of configuration for that seems more effort than it is worth (since black and ruff are easy to run locally without pre-commit).

dmdunla commented 1 year ago

I see. So, the CI test will block the merge if there are problems in the notebooks that do not conform to our use of nbstripout. And that should suffice for protecting commits with cruft we do not want.

ntjohnson1 commented 1 year ago

That's my current opinion. We call out pre-commit as an option for people that find it convenient, but if there's any issue configuring pre-commit then it isn't strictly a dev blocker.

ntjohnson1 commented 1 year ago

I changed my mind on this. I think adding the pre-commit package by default makes sense, then installing the hooks is optional. That probably provides the same flexibility as today but if people want the added ergonomics they can add the hooks. If someone complains because of some weird unexpected edge case we can backtrack.