Enable Ruff RET501 #115031
Enable Ruff RET501 #115031
Conversation
autinerd
requested review from
a team,
rytilahti,
syssi,
starkillerOG,
chemelli74,
epenet,
pavoni,
allenporter,
cgtobi,
emontnemery,
jbouwh,
janiversen,
bdraco,
dgomes,
pvizeli and
Danielhiversen
as code owners
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| @@ -82,13 +82,13 @@ class DateEntity(Entity, cached_properties=CACHED_PROPERTIES_WITH_ATTR_): | |||
| @final | |||
| def device_class(self) -> None: | |||
| """Return the device class for the entity.""" | |||
| return None | |||
| return | |||
There was a problem hiding this comment.
This is also wrong, I think. Properties should always return None explicitly since the return value will be consumed.
There was a problem hiding this comment.
From the runtime behavior there is no difference between return and return None. And as the function has a type annotation of -> None, there is nothing to be returned other than None.
>>> def a():
... return
...
>>> b = a()
>>> type(b)
<class 'NoneType'>
There was a problem hiding this comment.
Like I said, properties are always meant to have a return value for consumption and then we should return None explicitly, always. It's about reading the code, not about runtime behavior.
There was a problem hiding this comment.
I have it now as a noqa.
There was a problem hiding this comment.
I agree with @MartinHjelmare about making it explicit.
But adding a niqab to perfectly valid code is not a good way to go! that signals the code is somehow not correct!
There was a problem hiding this comment.
There are four options in total:
- Adding the type hints as the property functions are in the parent classes, then there is not only None as return type
- Adding noqa directives
- Allowing the implicit None in all functions which only return None
- Ignoring the rule altogether
When none of the first three are good, then I would say that I put this rule into ignore and we skip it.
There was a problem hiding this comment.
Another option is to raise it with RUFF, and either change the default or add an option to explicitely exclude:
- properties
- pytest fixtures
- methods/functions that override parents with a not-None return type
There was a problem hiding this comment.
The question here is, is this something that just we want, or is this a common pattern in the Python environment? (I checked the original flake8-return, and this plugin does not care about these cases as well)
There was a problem hiding this comment.
Following rebase... can you please keep return None on the properties?
There was a problem hiding this comment.
Ruff still doesn't like it. Either we change the declared return types to be the same as in the Entity class, or Ruff needs to be adjusted to take cached_property into account as well.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
@autinerd all the non-property issues should have been cleaned up now in preliminary PRs. Properties should be ignored when |
0.5.2 released... ready for rebase |
|
All changes are removed, but Ruff doesn't like it yet. |
I suggest to open a new issue in ruff, and add |
I couldn't find the issue in ruff... did you open it? |
PR merged on |
Proposed change
This enables the Ruff rule RET501: Do not explicitly
return Nonein function if it is the only possible return value.This fixes the occurrences as well, either by replacing
return Nonewithreturnor by adding type annotations to the functions.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: