Migrate tool configs to pyproject.toml, configure pre-commit.ci #342
Conversation
There was a problem hiding this comment.
Thanks @mfisher87! Overall the changes here look sensible to me. It looks like linting / testing didn't run here due to various restrictions we have in those GHA workflows. We should probably update those to include changes to more files. FWIW I would actually be okay always running linting and tests.
| @@ -78,6 +78,21 @@ requires = ["poetry>=0.12"] | |||
| build-backend = "poetry.masonry.api" | |||
|
|
|||
|
|
|||
| [tool.pytest] | |||
| filterwarnings = ["error::UserWarning"] | |||
There was a problem hiding this comment.
I realize this is just moving an existing setting around, but I'm curious what this is actually doing. Maybe causing UserWarnings to be elevated to errors?
There was a problem hiding this comment.
It's based on warnings.simplefilter it seems. Your speculation was spot on :)
As for "why", I'm not sure :)
Hm, yeah... good eye. I wonder when did this start happening? (EDIT: when someone opened a PR that doesn't change code or tests ;) ) It looks like we're not running pre-commit checks in CI at all? Maybe for another thread, but I've had a good user experience with and would like to use pre-commit.ci! |
This prevents repo changes from desyncing with the actions config, resulting in tests not running when we might expect them to.
Adjust YAML whitespace for consistency with our other YAML files
We're running linting as part of the test script here Lines 6 to 7 in e9c68ed But I agree, this isn't obvious at first
I'm also +1 for pre-commit (xref #282) |
|
Ah, thanks! I expected that they would be run through pre-commit so didn't think to search for direct invocations. I'll continue the conversation about linter changes over there. |
|
I updated our test workflow to remove path restrictions. Looking good now :) |
| ci: | ||
| autoupdate_schedule: "monthly" # Like dependabot | ||
| autoupdate_commit_msg: "chore: update pre-commit hooks" | ||
| autofix_prs: false # Comment "pre-commit.ci autofix" on a PR to trigger | ||
|
|
There was a problem hiding this comment.
I've not used pre-commit.ci before. I'm assuming this is correct and you'll handle whatever signup / authentication is needed to hook this repo into pre-commit.ci
There was a problem hiding this comment.
Yes, need to flip a switch on my other GitHub account. It will be just a minute...
MattF-NSIDC
changed the title
MattF-NSIDC
changed the title
|
It's on. It may not be watching this PR yet, but let's try... pre-commit.ci run EDIT: There goes :) |
|
Let's call fixes out of scope. New PR incoming. |
Pytest and Mypy can both be configured by
pyproject.toml.Flake8 can't, but Ruff can. I prefer Ruff anyway, so I opened #341.