-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop Python 3.8, 3.9 support to match napari and add precommit #35
Conversation
OH SHOOT, I guess with maintainer privileges I can directly push to this PR 😬 Lesson learned Normally it prompts me to instead make a branch and commit to the branch, so hopefully this doesn't break anything. During my review I was noticing the double quote style and wanted to lint as if it were single like in napari/napari. I'll leave it run the tests. Sorry if I break it 💀 Ok I canceled it. The break is entirely on me. Maybe pre-commit can't trigger this arg? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willingc Thank your for your persistence! I appreciate your explanation on the exclusion of the end-of-file linting for jinja files. Overall, the linting looks nice
I would like you to clarify for future maintainers: Did dropping 3.8/3.9 and adding 3.13 require these linting changes? If not, and we just changed python-versions, would the tests have been so troublesome?
One thing I noticed is that it uses double quote style. In napari/napari/pyproject.toml
there is
[tool.ruff.format]
quote-style = "single"
I don't think that's a blocker at all for this PR, but could be nice to include? Might be done with in precommit but I'm not sure yet
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.9.9
hooks:
- id: ruff-format
exclude: examples
args:
- "--quote-style=single"
- id: ruff
This reverts commit 2b3d3db.
I can't figure out how to properly pass the quote-style single in pre-commit, so I think I'll let the tests run and merge? I'm glad you are such an encouraging person and totally don't mind me figuring this all out in the fire so-to-speak |
@TimMonko I think the proper syntax is: |
Good questions. The changing of the python versions was not the issue. It was adding pre-commit to the repo and having it fix the existing files (which broke the template files end of file behavior). No worries on applying any changes. I can easily rebase to remove if needed though tests are passing. Feel free to iterate on it more. I'm at a conference today so will be slow to respond. |
Oh and please feel free to switch to single quotes as you prefer. I pulled the .precommit.config.yaml from the napari repo itself. |
@TimMonko Single quotes look good. Tests are passing. You can press the green button (just use the defaults when it pops up the next screen. |
@psobolewskiPhD great work sleuthing how to do that! I read that like 8 times earlier and definitely did not pick up how to format the arg correctly. @willingc again, great great work! I'll update PR description to explain the single quote addition. |
This PR replaces #22. It does the following: