New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix translation
typing
#66516
Fix translation
typing
#66516
Conversation
ccfc07b
to
64978a7
Compare
@@ -251,6 +252,7 @@ def _build_category_cache( | |||
translation_strings: dict[str, dict[str, Any]], | |||
) -> None: | |||
"""Extract resources into the cache.""" | |||
resource: dict[str, Any] | str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be able to be referenced from translation_strings
? And so we should update the type of the translation-strings
param instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type for translation_strings
is correct as far as I can tell. Any
is basically dict[str, Any] | str
there.
Shouldn't this be able to be referenced from translation_strings?
Yeah it is, but that's also the issue. Just from translation_strings
, resource
would be typed as dict[str, Any]
and thus wouldn't allow str
. The alternative would be to rename resource
in the third for loop to something like resource2
. That would allow for dedicated types.
Otherwise, we need the type definition here. The type inference still works like you would expect it to. I.e. inside the first the for ... translation_strings.values()
block, resource
is only dict[str, Any]
.
@@ -260,7 +262,8 @@ def _build_category_cache( | |||
resource_func = ( | |||
_merge_resources if category == "state" else _build_resources | |||
) | |||
new_resources = resource_func(translation_strings, components, category) | |||
new_resources: Mapping[str, dict[str, Any] | str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the preferred approach over casting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think type: ignore
with a type definition is slightly better here.
Once mypy
is able to infer resource_func
correctly, we could just remove the comment. With cast
we wouldn't even know that's the case, since the inferred type will be different from, but compatible with Mapping.
dict[str, dict[str, Any]] | dict[str, dict[str, Any] | str]
Proposed change
The return type for
_build_resources
should be changed todict[str, dict[str, Any] | str]
, since not all dictionary values are itself dictionaries. For examplecore/homeassistant/components/light/translations/en.json
Line 28 in 4c5d647
_build_category_cache
already has code to handle that case.core/homeassistant/helpers/translation.py
Lines 270 to 278 in 4c5d647
Type of change
Additional information
Checklist
black --fast 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
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: