search

scikit-hep / pyhf

pure-Python HistFactory implementation with tensors and autodiff
https://pyhf.readthedocs.io/
Apache License 2.0
282 stars 83 forks source link

handling pre-commit hooks for check-manifest #485

Open kratsg opened 5 years ago

kratsg commented 5 years ago

Question

We can't have a precommit hook for check-manifest as it fails harder than black which simply warns...

- repo: https://github.com/mgedmin/check-manifest
    rev: "0.39"
    hooks:
    - id: check-manifest
      language_version: python3.6

Relevant Issues and Pull Requests

See #481 which introduced check-manifest.

matthewfeickert commented 5 years ago

For additional context to future us, this is an issue because of me, as I have throw away test code that I have littered in that I don't really care enough to have in version control. check-manifest catches these and hard refuses to allow for a commit as a result. As pointed out, this also can happen for black (an example from @kratsg is that there might be a test submodule used that we might not want to black).

So perhaps in the future we should have a discussion if we want to enforce pre-commit hooks in the development flow and if a core dev doesn't want it allow them to turn it off by themselves. Or if we should keep the CI tests but just get rid of pre-commit hooks all together. I personally would say go for the first, but this should be discussed. Doing this, for the black example, the user would just add the submodule to the ignored file list for black.