Improve comment for helpers.entity.entity_sources #146529
Conversation
| """Get the entity sources.""" | ||
| """Get the entity sources. | ||
|
|
||
| Items are added to this dict by Entity.async_internal_added_to_hass and |
There was a problem hiding this comment.
I don't think we should have comments on place A saying that it is changed by B or C.
It usually makes it too easy for the comment to be stale and misleading in the future if the code changes. Without this, the developer just has to search for it or "find references", without getting mislead.
Why do you feel like this is needed? It seems easy too lookup already
There was a problem hiding this comment.
I think it's a really weird pattern to have callers mutate a dict returned to them from a function, and it was not obvious to me that's how this is meant to be used. I would expect a bare dict in global scope instead.
You're right the comment is likely to grow stale though, maybe it can just mention callers of helpers.entity.entity_sources are expected to mutate the dict as needed?
There was a problem hiding this comment.
I think this is ok. It's at least all in the same module.
There was a problem hiding this comment.
But now a wild PR appeared: #146549 🙈
There was a problem hiding this comment.
I think it's a really weird pattern to have callers mutate a dict returned to them from a function, and it was not obvious to me that's how this is meant to be used. I would expect a bare dict in global scope instead.
True, it is weird indeed.
You're right the comment is likely to grow stale though, maybe it can just mention callers of
helpers.entity.entity_sourcesare expected to mutate the dict as needed?
Yeah, I think describing what/how it should be done instead of where it is done would make it better.
There was a problem hiding this comment.
Something like "The returned dictionary can be mutated by callers"
There was a problem hiding this comment.
This is merged now, but I still believe that it should be updated to something that will not get stale and be misleading too easily. It will become stale as soon as #146549 is merged
Proposed change
Improve comment for
helpers.entity.entity_sources, the function simply returns an empty dictionary and it's not clear where that dictionary is mutated.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.To help with the load of incoming pull requests: