Organize translation into alphabetical order and remove upper case keys#249
Conversation
* Change entities to only use lower-case texts
steinmn
left a comment
steinmn
left a comment
There was a problem hiding this comment.
On the positive side, at least you caught this before we spent a lot of time changing all the keys to the native PascalCase keys from Zaptec. And with the exception of those trying out the first beta, charger_operation mode only changes once.
Readme needs to be updated in https://github.com/custom-components/zaptec/blob/master/README.md?plain=1#L67 and
https://github.com/custom-components/zaptec/blob/master/README.md?plain=1#L348-L353
Part of the motivation for moving from Charging to Connected_Charging was to keep the use of these values consistent within the integration. But now we're using connected_charging in the entities, while using Connected_Charging in api.py (L705,708 and 780). Wouldn't it be better to do the conversion to lower in the attribute type converters in zconst.py to keep it consistent across the integration?
|
I deliberately keep the two halves of the code separate; the api.py and the HA integration. One of the motivation is that if zaptec is ever to be included into mainline HA, the api.py needs to go to a separate library on pypi. That's why I chose to handle the lower-case thingy in the HA-side, not in the api part. I still think I'd like to use Zaptec's naming for the api-things, but as we've seen, we need a translation layer between the two policies. At least that was the initial thought... |
| @@ -53,7 +53,7 @@ def _update_from_zaptec(self) -> None: | |||
| """Update the entity from Zaptec data.""" | |||
| # Called from ZaptecBaseEntity._handle_coordinator_update() | |||
| state = self._get_zaptec_value() | |||
There was a problem hiding this comment.
| state = self._get_zaptec_value() | |
| state = self._get_zaptec_value().lower() |
Are we likely to get any cases where we have a str-value that shouldn't be converted to lower case, or should we consider including the conversion to lower inside the _get_zaptec_value() function?
There was a problem hiding this comment.
Good question. We might have string values, such as name or address that we'd like to preserve as is, so _get_zaptec_value() can't do it for all. But perhaps an argument preserve_str=True could be used for those cases where we do want to preserve it?
There was a problem hiding this comment.
That sounds like a good idea. On a related note, the states of installation_type and device_type are currently not translated (except Unknown?), so either they need to all be translated, or we need to preserve the capital letters for those.
There was a problem hiding this comment.
It's a dilemma on what to do with those enums that Zaptec provide and give (English) strings for. Should we replicate all in our translations, or should we use them as-is without any translations? Like these ones you mention here. Perhaps the best choice here is to remove Unknown and remove the lower-case stuff, using the English string from Zaptec as-is?
There was a problem hiding this comment.
For these particular ones, yes, I think it doesn't make much sense to translate them.
|
@steinmn I tried something else. I created a Do you think this is a good solution? I'm unsure if this is too complex, or if it helps. |
steinmn
left a comment
steinmn
left a comment
There was a problem hiding this comment.
I think this is probably the best solution so far, as long as we make sure to document why we need these lower case keys/strings.
Also need to add all the affected keys to the readme/breaking-changes
custom_components/zaptec/sensor.py
Outdated
|
|
||
| class ZaptecChargeSensor(ZaptecSensor): | ||
| class ZaptecSensorTranslate(ZaptecSensor): | ||
| """Sensor with strings intended for traslations.""" |
There was a problem hiding this comment.
| """Sensor with strings intended for traslations.""" | |
| """Sensor with strings intended for translations.""" |
Maybe also include a description for why we need this, ala
Several native Zaptec keys use capital letters.
Translation keys need to be lower case in order to pass hassfest.
| - Connected_Requesting: Waiting | ||
| - Connected_Charging: Charging | ||
| - Connected_Finished: Charge done | ||
| - connected_requesting: Waiting | ||
| - connected_charging: Charging | ||
| - connected_finished: Charge done |
There was a problem hiding this comment.
I think we need to list all the changed values, not just these 3.
2 participants
The purpose of this PR is to pass the validation tests (hassfest and hacs), however this fix is more intrusive than I'd like it to be.
The rule of no upper-case letters in translation key has implications. This means that the field values cannot have upper-case letters. This PR installs lower() in several locations to ensure this is the case. Notably, the value of
charger_operation_modehas changed once more, this time to only lower-case letters.