Enable more ruff rules #2855
Enable more ruff rules #2855
Conversation
TC006 will also make minor perf improvement > In some cases where cast() is used in a hot loop, this rule may also help avoid overhead from repeatedly evaluating complex type expressions at runtime.
|
|
||
| class PersonFieldsetListAdmin(admin.ModelAdmin[Person]): | ||
| fieldsets = [ | ||
| fieldsets = [ # noqa: RUF012 |
There was a problem hiding this comment.
I don't know, I feel it can catch future errors but sadly because of the ClassVar / TypeVar limitation, these few admin fields are not declared as Classvar. and trigger this rule.
What about ignoring for files matching the glob "*/admin*.py" ?
There was a problem hiding this comment.
These are tests, they are not real code. I don't see a valid reason to force this rule on tests files at all.
There was a problem hiding this comment.
Ye that make sense, but we probably still want it in the python plugin code for ex.
There was a problem hiding this comment.
So, ignore for tests using [tool.ruff.lint.per-file-ignores]?
|
|
||
| def get_django_metadata(model_info: TypeInfo) -> DjangoTypeMetadata: | ||
| return cast(DjangoTypeMetadata, model_info.metadata.setdefault("django", {})) | ||
| return cast("DjangoTypeMetadata", model_info.metadata.setdefault("django", {})) |
There was a problem hiding this comment.
Nit: Not opposed to this, but my first instinct is that these changes reduce readability.
From Ruff docs runtime-cast-value (TC006) https://docs.astral.sh/ruff/rules/runtime-cast-value/#why-is-this-bad
In some cases where
cast()is used in a hot loop, this rule may also help avoid overhead from repeatedly evaluating complex type expressions at runtime.
I guess that's a point sometimes. But doesn't really apply when the type expression is a simple type reference.
There was a problem hiding this comment.
Nit: Not opposed to this, but my first instinct is that these reduce readability.
Agree, but I'm even wondering if we should really be using cast in these cases, a type: ignore makes more sense here for example and completely removes the function call
The perf argument does seem compelling since we might be in hot loops when running mypy
I have made things!
(Review commit per commit should be easier)
Followup to #2852, adding a few ruff rules I often use.