zarr-python icon indicating copy to clipboard operation
zarr-python copied to clipboard

Published 20 hours ago • verified publisher icon zarr-developers

Add pre-commit configuration

Open joshmoore opened this issue 3 years ago • 5 comments

Sorry I missed the linting in the dev guide. I can recommend https://pre-commit.com/ for these type of checks, we use it heavily over at tskit: https://github.com/tskit-dev/tskit/blob/main/.pre-commit-config.yaml

Originally posted by @benjeffery in https://github.com/zarr-developers/zarr-python/issues/974#issuecomment-1055670808

joshmoore avatar Mar 04 '22 00:03 joshmoore

Hey @joshmoore , can i work on this? Could you please let me know what all pre-commit checks are required in zarr?

parthxtripathi avatar Mar 23 '22 04:03 parthxtripathi

Hi @parthxtripathi.

can i work on this?

That'd be great. Sure.

Could you please let me know what all pre-commit checks are required in zarr?

I imagine we will need to go through this iteratively and find the checks which can be added without too many changes, and then separate out those checks which require more work.

My starting point is usually:

https://github.com/ome/ome-zarr-py/blob/master/.pre-commit-config.yaml

but welcome to hear/see suggestions from others.

joshmoore avatar Mar 23 '22 13:03 joshmoore

Thank you for the details @joshmoore ! Just to confirm, this issue requires adding the pre-commit configuration to zarr-python right? As a first step, should I add the configuration available in ome-zarr-py to my fork and create a PR? Then I can iteratively work on the configuration!

parthxtripathi avatar Mar 25 '22 06:03 parthxtripathi

Having been through this process over at tskit I can recommend adding a minimal pre-commit configuration in a PR, then adding additional rules one-by-one in separate PRs. This makes it clear what changes to the code/formatting are from which rules and allows discussion of each one. Some (e.g. black, bugbear etc) can cause a lot of changes, depending on the existing state of the code.

You'll also need some hand-holding in the dev docs, here's ours: https://tskit.dev/tskit/docs/stable/development.html#pre-commit-checks

benjeffery avatar Mar 25 '22 12:03 benjeffery

I can recommend adding a minimal pre-commit configuration in a PR, then adding additional rules one-by-one in separate PRs

Thanks, @benjeffery. @parthxtripathi, following that logic, I could see this as an ordering of the hooks:

Should require minimal changes (if any)

  • https://github.com/PyCQA/flake8

Hopefully only a few changes

  • https://github.com/pre-commit/mirrors-mypy
  • https://github.com/adrienverge/yamllint.git
  • https://github.com/asottile/pyupgrade

Moderate changes

  • https://github.com/pre-commit/pre-commit-hooks
  • https://github.com/asottile/seed-isort-config
  • https://github.com/PyCQA/isort

Requires lots of changes

  • https://github.com/psf/black

Anyone else have hooks they would like to suggest?

joshmoore avatar Mar 25 '22 13:03 joshmoore

I think we've likely got the gist of this. Closing for now (but definitely looking to the day when black will be a part of pre-commit :smile:)

joshmoore avatar Nov 23 '22 21:11 joshmoore