Ruff #175
Ruff #175
Conversation
cumulus_library/statistics/psm.py
Outdated
There was a problem hiding this comment.
All the noqa statements in this file are just 'PsmPy is doing something under the hood I don't want to debug unless I can open a PR against it to fix it'. Still waiting on that.
dogversioning
force-pushed
the
mg/ruff
branch
from
c4b251c
to
29181f0
Compare
dogversioning
force-pushed
the
mg/ruff
branch
from
29181f0
to
664ebef
Compare
.pre-commit-config.yaml
Outdated
| - id: ruff | ||
| args: [--fix] |
There was a problem hiding this comment.
Ah here's the philosophical rub. We've talked about how appropriate it is to automatically fix bad code for the developer in a way that they aren't brought into the loop so they can learn to be better.
This does bring them into the loop a little bit, by not auto-committing the results at least. So I'd like to keep it that way, if/when we make the auto-formatting auto-committed.
I'd personally prefer to keep such checks out of the commit hooks altogether because of my WIP-commit-heavy workflow. I assume your workflow is different.
Anyway, we don't need to agree on this kind of thing. Just raising the point again.
There was a problem hiding this comment.
ok, per this and our chat off PR about this last night, I did some digging and found that pre-commit supports hooks at basically every lifecycle stage, so i've moved this check to pre-push. It didn't look like it is installed normally, so you have to specify the stage with pre-commit install --hook-type pre-push, but it then does the linting only on push to remote.
There was a problem hiding this comment.
Interesting... OK - we have the pre-commit instructions in our CONTRIBUTING.md docs usually (I think that's what we named them). - maybe those need updating.
It's annoying that pre-commit isn't more turnkey, but fine 😄
There was a problem hiding this comment.
ah yep, meant to do that and spaced out - bumped the docs.
.pre-commit-config.yaml
Outdated
| language_version: python3.9 | ||
| - id: ruff | ||
| args: [--fix] | ||
| - id: ruff-format |
There was a problem hiding this comment.
On the other hand, formatting I find it awkward to push in front of the dev - they didn't do anything wrong really. So this I'd like to see return to auto-commit status at some point, once we trust it (which off-PR we talked about as the reason for this change).
tests/test_cli.py
Outdated
| @@ -29,7 +27,8 @@ def open(self, *args, **kwargs): | |||
| if str(args[0]).endswith(".bsv"): | |||
| args = ( | |||
| Path( | |||
| f"./tests/test_data/mock_bsvs/{str(args[0]).rsplit('/', maxsplit=1)[-1]}" | |||
| "./tests/test_data/mock_bsvs/", | |||
| f"{str(args[0]).rsplit('/', maxsplit=1)[-1]}", | |||
There was a problem hiding this comment.
nit: I think you can drop the surrounding f-string if you're breaking it off to its own Path() arg
| def test_cli_path_mapping( | ||
| mock_load_json, mock_path, tmp_path, args, raises, expected | ||
| ): # pylint: disable=unused-argument | ||
| def test_cli_path_mapping(mock_load_json, mock_path, tmp_path, args, raises, expected): # pylint: disable=unused-argument |
There was a problem hiding this comment.
This feels like a long line for ruff - curious why it allows it here.
There was a problem hiding this comment.
#noqa/pylint codes seem to be allowed to break line length restricitions. i dont hate it.
| *_, last_new = f | ||
| last_new = json.loads(last_new) |
tests/test_conftest.py
Outdated
| except Exception: | ||
| last_ref = first_ref |
There was a problem hiding this comment.
What's with this exception here? That's new behavior yeah? Is there a more specific exception class to reference?
There was a problem hiding this comment.
if you try to unpack an iterator and its empty, you get an error - the docs didn't specify which. I can do some research.
There was a problem hiding this comment.
Speaking of, it's interesting to me that ruff doesn't complain about a loose Exception catch. Which... I don't mind. There are plenty of valid reasons for it.
Can you check if it would complain about a bare except? Like if you did except: just alone -- I would want it to treat that as a flaggable error (and probably not an auto-fix one).
There was a problem hiding this comment.
it did, that's how we ended up here in the first place.
There was a problem hiding this comment.
There wasn't a bare-except here before. But maybe during PR work you had one?
There was a problem hiding this comment.
yeah, it happened midstream
dogversioning
force-pushed
the
mg/ruff
branch
from
ba346ef
to
8f8c15f
Compare
This PR makes the following changes:
Checklist
docs/) needs to be updated