matthewfeickert commented 3 years ago

Description

Given that we're taking advantage of pre-commit.ci it is probably also worth considering to use the pre-commit hooks from https://github.com/pre-commit/pre-commit-hooks ("Some out-of-the-box hooks for pre-commit.").

Modifying the version that @henryiii has in his Level Up Your Python section on pre-commit to v3.4.0 and to use check-json, check-toml, and check-xml (currently on branch ci/add-pre-commit-hooks-hooks)

repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.4.0
    hooks:
    - id: check-added-large-files
    - id: check-case-conflict
    - id: check-merge-conflict
    - id: check-symlinks
    - id: check-json
    - id: check-yaml
    - id: check-toml
    - id: check-xml
    - id: debug-statements
    - id: end-of-file-fixer
    - id: mixed-line-ending
    - id: requirements-txt-fixer
    - id: trailing-whitespace

passes almost everything right out of the box. The one thing it is changing is end-of-file-fixer is trying to change the schemas and validation files that were generated with no ending newline

$ git diff origin/master --name-only 
.pre-commit-config.yaml
docs/examples/notebooks/binderexample/meas.xml
docs/examples/notebooks/binderexample/meas_channel1.xml
src/pyhf/schemas/HistFactorySchema.dtd
tests/contrib/hypotestresults.json
validation/data/1bin_histosys.json
validation/data/2bin_2channel_coupledhisto.json
validation/data/2bin_2channel_coupledshapefactor.json
validation/data/2bin_histosys_example2.json
validation/multibin_histfactory/config/HistFactorySchema.dtd
validation/multibin_histfactory_p0/config/HistFactorySchema.dtd
validation/multichan_coupledhistosys_histfactory/config/HistFactorySchema.dtd
validation/multichan_coupledoverall_histfactory/config/HistFactorySchema.dtd
validation/multichannel_histfactory/config/HistFactorySchema.dtd
validation/overallsys_histfactory/config/HistFactorySchema.dtd
validation/shared_nuispar_across_types/config/HistFactorySchema.dtd
validation/xmlimport_input/config/HistFactorySchema.dtd
validation/xmlimport_input/config/examples/HistFactorySchema.dtd
validation/xmlimport_input/log
validation/xmlimport_input/results/example_results.table
validation/xmlimport_input2/config/HistFactorySchema.dtd
validation/xmlimport_input3/config/examples/HistFactorySchema.dtd

@kratsg @lukasheinrich if you like the idea of thes hooks in general should we avoid end-of-file-fixer or try to setup ignores? Or should we just let it change things, creating large useless diffs?

henryiii commented 3 years ago

Also from here: https://scikit-hep.org/developer/style#pre-commit

henryiii commented 3 years ago

If you have a style only change, you can list it in .git-blame-ignore-revs (often, just using the whitespace ignore to blame is enough). https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame

It's not picked up by GitHub, yet, sadly.

henryiii commented 3 years ago

Though, for generated files, I'd probably just turn off the end-of-file fixer for those extensions.

matthewfeickert commented 3 years ago

Though, for generated files, I'd probably just turn off the end-of-file fixer for those extensions.

Ah, I didn't realize you could use excludes in pre-commit. Thanks @henryiii!

repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.4.0
    hooks:
    - id: check-added-large-files
    - id: check-case-conflict
    - id: check-merge-conflict
    - id: check-symlinks
    - id: check-json
    - id: check-yaml
    - id: check-toml
    - id: check-xml
    - id: debug-statements
    - id: end-of-file-fixer
      # exclude generated files
      exclude: validation/|\.dtd$|\.json$|\.xml$
    - id: mixed-line-ending
    - id: requirements-txt-fixer
    - id: trailing-whitespace
      # exclude generated files
      exclude: validation/|\.dtd$|\.xml$

works nicely now. :+1:

scikit-hep / pyhf

Add pre-commit-hooks checks to pre-commit-hooks #1275

Description