MAINT: Ruff safe-fixes - #235
Conversation
Signed-off-by: Pradyot Ranjan <99216956+pradyotRanjan@users.noreply.github.com>
ev-br
left a comment
ev-br
left a comment
There was a problem hiding this comment.
LGTM modulo a question about nested context managers.
If the fix for that is onerous, fine to keep as is, too.
| float64, | ||
| complex128, | ||
| ) | ||
| from .._dtypes import ( |
There was a problem hiding this comment.
nit: can merge this import together with the one just above?
There was a problem hiding this comment.
Ruff does this as a default behaviour now, but we can revert this by setting combine-as-imports = true. See docs: https://docs.astral.sh/ruff/settings/?utm_source=chatgpt.com#lint_isort_combine-as-imports
There was a problem hiding this comment.
Fixed in the latest commit.
| from .._devices import CPU_DEVICE, ALL_DEVICES, Device | ||
| from .._info import __array_namespace_info__ | ||
| from .._devices import ALL_DEVICES, CPU_DEVICE, Device | ||
| from .._dtypes import bool as xp_bool |
There was a problem hiding this comment.
Thtat's the second time from .._dtypes import ... as ... is separated from the same import. Is it The Right Way To Do It, according to the Supreme Lint Authority now?
Fine by me if so, we mere mortals will adapt (and that's not that big of a deal anyway, just unexpected).
There was a problem hiding this comment.
Ruff does this as a default behaviour now, but we can revert this by setting combine-as-imports = true. See docs: https://docs.astral.sh/ruff/settings/?utm_source=chatgpt.com#lint_isort_combine-as-imports
Same.
| with ArrayAPIStrictFlags(api_version='2023.12'): | ||
| with pytest.raises(TypeError): | ||
| xp.mean(a) | ||
| with ArrayAPIStrictFlags(api_version='2023.12'), pytest.raises(TypeError): |
There was a problem hiding this comment.
This I've to admit the original is more readable because it clearly separates a setting of flags and an assertion (which just so happens to be a context manager, too).
What would it take to convince ruff to leave this alone? # noqa comments are line noise, let's not add them.
There was a problem hiding this comment.
We can ignore SIM117 globally in ruff.toml
There was a problem hiding this comment.
Fixed in the latest commit.
| _dtype: DType | ||
| _device: Device | ||
| __slots__ = ("_array", "_dtype", "_device", "__weakref__") | ||
| __slots__ = ("__weakref__", "_array", "_device", "_dtype") |
There was a problem hiding this comment.
Huh. I actually thought that __slots__ are ordered and the order translates directly to the order of elements of a corresponding C struct. Apparently not, and I actually don't know why I thought this. TIL.
(this is a note-to-self, not a request to change anything)
| *, | ||
| keepdims: bool = False, | ||
| ord: float | Literal["fro", "nuc"] | None = "fro", | ||
| ) -> Array: # noqa: F821 |
There was a problem hiding this comment.
Oh great, thanks for this! #noqa: comments are line noise.
| get_array_api_strict_flags, | ||
| requires_api_version, | ||
| requires_data_dependent_shapes, | ||
| ) |
There was a problem hiding this comment.
No idea why it keeps reformatting imports from a single line of 101 symbol to a column. It can't be just line length, because there are longer lines just below.
Just a note-to-self, not a request.
Signed-off-by: Pradyot Ranjan <99216956+pradyotRanjan@users.noreply.github.com>
There was a problem hiding this comment.
LGTM and seems uncontrovesial, lets' get this in. I'll squash-merge so that there's a single commit to .git-blame-ignore-revs per #228 (comment)
Thanks @prady0t
2 participants
#234 #228