Update pre-commit configuration to use Ruff for linting and formatting#1031
Update pre-commit configuration to use Ruff for linting and formatting#1031
Conversation
cuza
force-pushed
the
cuza/replacing-flake8,-black,-pyupgrade,etc-with-ruff
branch
from
7002922 to
afcf3b5
Compare
EvanKrall
left a comment
EvanKrall
left a comment
There was a problem hiding this comment.
It would be interesting to upgrade black to 24.8.0 first, and then double-check that ruff isn't doing anything black wouldn't do.
This is more palatable than it was originally; most of the changes fall into a few categories:
- added or removed blank lines (not a problem in
git blame) - things that black should already be doing (splitting mock.patch args onto multiple lines with commas)
- changes that we should make anyway (combining
f"a b c" f"d e f"intof"a b cd e f"
|
|
||
| def make_action(**kwargs): | ||
| kwargs.setdefault("name", "action"), | ||
| (kwargs.setdefault("name", "action"),) |
There was a problem hiding this comment.
we should probably just not have the trailing comma on these lines
| hooks: | ||
| - id: black | ||
| args: [--target-version, py38] | ||
| files: ^tests/.*\.py$ No newline at end of file |
There was a problem hiding this comment.
I think this shows that we want to keep end-of-file-fixer
There was a problem hiding this comment.
I got rid of it in my pr, I think this is covered by W292
https://docs.astral.sh/ruff/rules/missing-newline-at-end-of-file/
| pod_annotations=mock_k8s_action_run.command_config.annotations, | ||
| service_account_name=mock_k8s_action_run.command_config.service_account_name, | ||
| ports=mock_k8s_action_run.command_config.ports, | ||
| ), mock_get_cluster.return_value.create_task.calls |
There was a problem hiding this comment.
Maybe just delete the , mock_get_cluster.return_value.create_task.calls -- I assume this was meant to have the list of calls be included into the assertion error. If you delete that, then ruff shouldn't want to change the indentation here.
With an assert statement, extra data that is passed in after the comma will be included into the AssertionError:
>>> assert 1==2, "hello"
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AssertionError: hello
but I'm pretty sure that doesn't work with assert_called_once_with:
>>> from unittest import mock
>>> a = mock.Mock()
>>> a.b.assert_called_once_with(3), "hello"
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3.8/unittest/mock.py", line 924, in assert_called_once_with
raise AssertionError(msg)
AssertionError: Expected 'b' to be called once. Called 0 times.
| with mock.patch( | ||
| "tron.kubernetes.PyDeferredQueue", | ||
| autospec=True, | ||
| ), mock.patch( |
There was a problem hiding this comment.
Surprised this isn't already formatted like this. How old of black are we running?
There was a problem hiding this comment.
Looks like black 24.8.0 (last version of black that supports py38) also formats it this way.
| ), mock.patch( | ||
| "tron.config.static_config.load_yaml_file", | ||
| autospec=True, | ||
| ), mock.patch("tron.utils.logreader.get_superregion", autospec=True, return_value="fake"), mock.patch( |
There was a problem hiding this comment.
I'd add the trailing comma here after return_value="fake"
| "tron.utils.logreader.S3LogsReader", autospec=True | ||
| ) as mock_s3_reader: | ||
|
|
||
| ), mock.patch("tron.utils.logreader.S3LogsReader", autospec=True) as mock_s3_reader: |
There was a problem hiding this comment.
I'd add trailing comma after autospec=True
| constraints=[["an attr", "an op", "a val"]], | ||
| docker_parameters=[{"key": "init", "value": "true"}], | ||
| **self.other_task_kwargs, | ||
| ), mock_get_cluster.return_value.create_task.calls |
3 participants
Replaced pre-commit hooks
flake8,black,pyupgrade, etc. withrufffor linting and formatting.