Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add language selection for entity naming in WolfLink #126764

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

alexdev03
Copy link
Contributor

Breaking change

This pr uses the new locale system of wolf-comm 0.0.10, so it's recommended to reinstall the integration

Proposed change

A new locale input parameter is now asked when logging in into the integration, it can be something like (en, es, de, it, etc)
The locale format doesn't follow a common standard for locales, it uses Wolf's website language system.
Entities names are now also translated based on the locale.
If there are 2 entities with the same name but different group. now both of them will be loaded. Previously only one of them was loaded.
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @adamkrol93, @mtielen, mind taking a look at this pull request as it has been labeled with an integration (wolflink) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of wolflink can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign wolflink Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

Copy link
Contributor

@mtielen mtielen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexdev03 Thanks for making it more international. I think it would be better to use LANGUAGE as users know what it is I don't think they know what to fill in at LOCALE

homeassistant/components/wolflink/const.py Outdated Show resolved Hide resolved
tests/components/wolflink/const.py Outdated Show resolved Hide resolved
tests/components/wolflink/const.py Show resolved Hide resolved
tests/components/wolflink/test_config_flow.py Outdated Show resolved Hide resolved
@MartinHjelmare
Copy link
Member

Will the locale also affect the entity state values in Home Assistant?

@MartinHjelmare MartinHjelmare marked this pull request as draft September 25, 2024 18:24
@MartinHjelmare MartinHjelmare changed the title WolfLink - Using new locale system Use new locale system in WolfLink Sep 25, 2024
@alexdev03
Copy link
Contributor Author

Will the locale also affect the entity state values in Home Assistant?

Yes, previously they were in german

@MartinHjelmare
Copy link
Member

We've previously declined other integrations from changing the language of returned values from API. One reason is that it breaks automations. I'll take it up with the Core team.

If there are any mitigating factors that we should take with us when discussing this, you're welcome to explain the situation further. Thanks!

@MartinHjelmare MartinHjelmare added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Sep 25, 2024
@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

Will the locale also affect the entity state values in Home Assistant?

Yes, previously they were in german

This also not correct the currently they translate to English using HA Language system.
I run my home assistant in English and you see the values in English. If you run it in German you see it in German. The default language is German due to Wolf API results. @alexdev03 have you checked the translations keep working if you are changing the values also?

See here a screenshot of the current productive version of the integration in English and in German:
image
image

@alexdev03
Copy link
Contributor Author

Sorry I misunderstood, states are not translated with the api.
Only names and Entity IDs.

@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

Thanks @alexdev03 Yes I just tested it with using Italian and can confirm states don't get translated via the api only the entities created:
image

Btw I think at the login to Wolf I still think we should call it language instead of Locale as it for a basic user I think not self explaining

@alexdev03
Copy link
Contributor Author

Thanks @alexdev03 Yes I just tested it with using Italian and can confirm states don't get translated via the api only the entities created: image

Btw I think at the login to Wolf I still think we should call it language instead of Locale as it for a basic user I think not self explaining

Yes I'm fixing conversations right now.

@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

Thanks @alexdev03 Yes I just tested it with using Italian and can confirm states don't get translated via the api only the entities created: image
Btw I think at the login to Wolf I still think we should call it language instead of Locale as it for a basic user I think not self explaining

Yes I'm fixing conversations right now.

Great only problem I see with the new group feature is duplicated entities which are listed in Wolf's API on different places but they are the same sensor. This is why in the past we only took one if they had the same name. In my case it's 6 duplicate entities.

@alexdev03
Copy link
Contributor Author

image
This is input menu.

@alexdev03
Copy link
Contributor Author

Thanks @alexdev03 Yes I just tested it with using Italian and can confirm states don't get translated via the api only the entities created: image
Btw I think at the login to Wolf I still think we should call it language instead of Locale as it for a basic user I think not self explaining

Yes I'm fixing conversations right now.

Great only problem I see with the new group feature is duplicated entities which are listed in Wolf's API on different places but they are the same sensor. This is why in the past we only took one if they had the same name.

Could you give me some examples please?

@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

See here some examples:

image
image

@alexdev03
Copy link
Contributor Author

You mean something like this?
image

@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

You mean something like this? image

Yes they are like that as they are twice in the api return across different sections

@alexdev03
Copy link
Contributor Author

I prefer duplicate values instead of missing values. Maybe I could do a check somewhere in Wolf-Comm and prevent the problem.

@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

I prefer duplicate values instead of missing values. Maybe I could do a check somewhere in Wolf-Comm and prevent the problem.

You are actually missing more value's if you look into responds json-structure. Much more data available of your system is available then what currently is included via the lib as it only is focused on user heating systems values You technically have the full admin values etc also available to you but they are filtered out as they are in a different part of the json. I guess it would be good to make sure we don't get duplicates from the lib as some systems have lot of parameters.

@alexdev03
Copy link
Contributor Author

I prefer duplicate values instead of missing values. Maybe I could do a check somewhere in Wolf-Comm and prevent the problem.

You are actually missing more value's if you look into responds json-structure. Much more data available of your system is available then what currently is included via the lib as it only is focused on user heating systems values You technically have the full admin values etc also available to you but they are filtered out as they are in a different part of the json. I guess it would be good to make sure we don't get duplicates from the lib as some systems have lot of parameters.

I should have fixed the duplicate problem directly in wolf-link, I think we could keep the fix here in wolf-link and also it would be better to fix that also in wolf-comm

Copy link
Contributor

@mtielen mtielen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alexdev03 looks good for me. Let's see what the HA core team says. I've tested it with my system and works.

@alexdev03
Copy link
Contributor Author

alexdev03 commented Sep 25, 2024

Thanks @alexdev03 looks good for me. Let's see what the HA core team saids. I've tested it my system and works.

Nice.

MOCK_RESPONSE is an example of this https://www.wolf-smartset.com/js/localized-text/text.culture.en.js just with only a few values.
Unfortunately not all key-value entries are usable since that is not a valid json and I had to parse it in a strange way. Most of them should be usable, better than nothing.

@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

@home-assistant rename Add language selection for entity naming in WolfLink

@home-assistant home-assistant bot changed the title Use new locale system in WolfLink Add language selection for entity naming in WolfLink Sep 25, 2024
@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

Thanks @alexdev03 looks good for me. Let's see what the HA core team saids. I've tested it my system and works.

Nice.

MOCK_RESPONSE is an example of this https://www.wolf-smartset.com/js/localized-text/text.culture.en.js just with only a few values. Suddenly not all key-value entries are usable since that is not a valid json and I had to parse it in a strange way. Most of them should be usable, better than nothing.

Yeah their API is terrible specially with all the numbers etc and the mapping is even worse. I am working on option to writevalues back but it's a lot of debugging. This way we can introduce switches etc. Btw you can probably put this pull request into Open because it's currently in draft and doesn't change the values so it will not break automations.

@alexdev03 alexdev03 marked this pull request as ready for review September 25, 2024 20:31
@MartinHjelmare
Copy link
Member

MartinHjelmare commented Sep 25, 2024

We want entity names and entity_ids to be translated using the Home Assistant translation system.

https://developers.home-assistant.io/docs/core/entity#entity-naming

What's the point of allowing the user to set a language for the integration with this in mind?

@MartinHjelmare MartinHjelmare marked this pull request as draft September 25, 2024 21:52
@mtielen
Copy link
Contributor

mtielen commented Sep 25, 2024

We want entity names and entity_ids to be translated using the Home Assistant translation system.

https://developers.home-assistant.io/docs/core/entity#entity-naming

What's the point of allowing the user to set a language for the integration with this in mind?

I understand the wish from a HA point of view. In this case I don't think it would every be practical or work well.
The API is complex. To fully translate it manually you would need Wolf's dictionary with thousands of parameters which is not freely available. For every different type of system they have different parameter id's and mappings. This feature would make it happen without overcomplicating it. It's a practical solution for this integration as every user would get the entities the delivered based on their personal system setup and based on their chosen language.

@MartinHjelmare
Copy link
Member

We only need to translate the existing entity names.

@alexdev03
Copy link
Contributor Author

We only need to translate the existing entity names.

This pr not only translates entities but also allows to have entities with the same name but different group. For example with the previous system If I had 2 heat pumps I could only see the power consumption of a single heat machine since the name was equal. Now the name is not equal since it's formed by the group, in this case, heat pump 1 or heat pump 2, and then the entire name would be heat pump 1 power consumption and heat pump 2 power consumption. This pr fixes 2 problems, and can't be divided. Previous systems are not compatible since entity names don't start with the entity group. So it's not possible to just "translate the existing entity names". I understand that the best thing would be to have locales in a json file, but that would be too complicated. Wolf has 30 locales and each locale file weight like 450kb, that would mean that this integration would need to handle 13MB of locales. I think that is too much work and as mtielen said, that is a practical solution.

@mtielen
Copy link
Contributor

mtielen commented Sep 26, 2024

We only need to translate the existing entity names.

The entities are not fixed as they are based on the system configuration of the heating setup Like on 1 or more pumps, solar or no solar, oil, gas, heatpump and the connected optional components and sensor which are installed. The API returns these values based on the system and this is how the entities then are created.

@MartinHjelmare
Copy link
Member

I don't believe we'll accept this PR but I'll check with the core team.

@mtielen
Copy link
Contributor

mtielen commented Sep 26, 2024

I don't believe we'll accept this PR but I'll check with the core team.

It's sad to hear that if that would be the case. It's a practical solution for a small integration like this one. But if this is the core team decision then so be it.

@mtielen mtielen mentioned this pull request Sep 26, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change cla-signed has-tests integration: wolflink new-feature Quality Scale: No score second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. small-pr PRs with less than 30 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants