Skip to content
This repository was archived by the owner on Dec 1, 2025. It is now read-only.

Add .pre-commit-config.yaml#308

Merged
evamaxfield merged 5 commits into
mainfrom
unknown repository
Sep 17, 2021
Merged

Add .pre-commit-config.yaml#308
evamaxfield merged 5 commits into
mainfrom
unknown repository

Conversation

@tlambert03

Copy link
Copy Markdown
Collaborator

Description

This adds a .pre-commit-config.yaml with settings that mimic what's currently in tox.ini
This is opt-in... it doesn't do anything unless you run pre-commit install. But if you do, then it will add a pre-commit hook to your local git repo make sure that the code you're about to check in will pass linting on CI before checking it in.

I stopped short of using this to actually perform the CI, but if you'd like to keep all of your lint settings in one place, I could generally change the linting commands to be pre-commit run --all-files.

also note: with this, you could turn on https://pre-commit.ci/ ... which would run this as a part of PR tests, and would also fix some things automatically for contributors (adding a new commit to the PR).

one more note: as is, this will modify a few errors (like black errors or isort errors) if it finds them, but it won't actually check them in. It will just tell you that there were errors (and prevent you from committing them) and leave the fixes in your unstaged area.

Pull request recommendations:

  • Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
  • Link to any relevant issue in the PR description. Ex: Resolves [Fix release #12], adds tiff file format support
  • Provide relevant tests for your feature or bug fix.
  • Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

@codecov-commenter

codecov-commenter commented Sep 17, 2021

Copy link
Copy Markdown

Codecov Report

Merging #308 (0fef9df) into main (0304c50) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 0fef9df differs from pull request most recent head fc7262b. Consider uploading reports for the commit fc7262b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
- Coverage   95.77%   95.76%   -0.02%     
==========================================
  Files          35       35              
  Lines        2628     2620       -8     
==========================================
- Hits         2517     2509       -8     
  Misses        111      111              
Impacted Files Coverage Δ
aicsimageio/exceptions.py 100.00% <ø> (ø)
aicsimageio/readers/reader.py 95.15% <ø> (+2.25%) ⬆️
aicsimageio/writers/writer.py 100.00% <ø> (+11.11%) ⬆️
aicsimageio/tests/conftest.py 92.30% <0.00%> (-3.85%) ⬇️
aicsimageio/tests/readers/test_czi_reader.py 96.29% <0.00%> (-3.71%) ⬇️
aicsimageio/readers/ome_tiff_reader.py 96.94% <0.00%> (-0.77%) ⬇️
aicsimageio/readers/czi_reader.py 95.43% <0.00%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0304c50...fc7262b. Read the comment docs.

@evamaxfield evamaxfield added the admin Various administrative tasks for the package label Sep 17, 2021

@evamaxfield evamaxfield left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! I probably won't use this personally but I know @AetherUnbound uses pre-commit so you may be the best to say if this is alright or not

@toloudis toloudis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Only minor concern is duplicated settings, that we'd want to change in two places if we ever change them?

@tlambert03

Copy link
Copy Markdown
Collaborator Author

This looks great. Only minor concern is duplicated settings, that we'd want to change in two places if we ever change them?

yep, if you want to you could make tox.ini and/or the github lint test be:

deps = pre-commit
command = pre-commit run --all-files

(or just remove them altogether and use pre-commit.ci). This all depends on your preferred workflow as a group though. didn't want to impose on a core part of your CI.

@AetherUnbound AetherUnbound left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for the contribution 😄 I do support replacing the tox command with pre-commit run --all-files. I've done that in other projects and it's nice to just have one place for lint configuration.

@evamaxfield

Copy link
Copy Markdown
Collaborator

If @AetherUnbound says replace tox command with precommit I trust. Would also alleviate @toloudis 's comment on multiple files to change the same configuration.

@evamaxfield

Copy link
Copy Markdown
Collaborator

Side note: because the pre-commit has "rev" tags, does that mean we should get rid of the deps from setup.py? Sorry learning pre-commit here 😅

@tlambert03

Copy link
Copy Markdown
Collaborator Author

because the pre-commit has "rev" tags, does that mean we should get rid of the deps from setup.py?

technically, yes, you could remove them entirely from setup.py. pre-commit actually makes it's own little virtual environment to analyze the code, it doesn't use your active virtual env... personally, I tend to leave those deps in my dev_requirements, because i still want to have them available in my environment for the IDE and stuff.
maybe I'll make another "full adoption" commit. then you can have a look and we can revert back if you have concerns.

@tlambert03

Copy link
Copy Markdown
Collaborator Author

here's the commit using it for other stuff: 0fef9df

@AetherUnbound

Copy link
Copy Markdown
Collaborator

Yea great question @JacksonMaxfield - I also leave them in the dev requirements for the same reason @tlambert03 mentioned!

@evamaxfield

Copy link
Copy Markdown
Collaborator

All sounds good to me! Last note then is that I think we may need to add something to the setup instructions in CONTRIBUTING about setting up pre-commit / installing pre-commit

After that, happy to merge 👍

@tlambert03

Copy link
Copy Markdown
Collaborator Author

added a note, but happy to reword or move it!

  1. (Optional) If you'd like to have linting checks run automatically prior to
    every git commit, then you can install a commit-hook that runs
    pre-commit, as follows:
  pip install pre-commit
  pre-commit install

@evamaxfield evamaxfield merged commit 57e8375 into AllenCellModeling:main Sep 17, 2021
@evamaxfield

Copy link
Copy Markdown
Collaborator

Perfect. Thanks! Merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

admin Various administrative tasks for the package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants