core icon indicating copy to clipboard operation
core copied to clipboard

Add sensor language option to sma integration

Open frimtec opened this issue 2 years ago • 3 comments

Proposed change

Added configuration option to SMA Solar integration to choose sensor value language. If configured language is not supported by device, it fallsback to English. sma-lang-option The change updates dependency pysma from 0.7.3 to 0.7.4 which includes the following change: https://github.com/kellerza/pysma/pull/117.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] 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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/26544

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the [development checklist][dev-checklist]
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

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

  • [x] Documentation added/updated for [www.home-assistant.io][docs-repository]

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

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

To help with the load of incoming pull requests:

  • [ ] I have reviewed two other [open pull requests][prs] in this repository.

frimtec avatar Mar 06 '23 07:03 frimtec

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

Code owner commands

Code owners of sma 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 sma Removes the current integration label and assignees on the pull request, add the integration domain after the command.

home-assistant[bot] avatar Mar 06 '23 07:03 home-assistant[bot]

Dear @kellerza or @rklomp, is there any chance to get this PR approved?

frimtec avatar Mar 17 '23 17:03 frimtec

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Mar 20 '23 09:03 home-assistant[bot]

Dear @frenck

Thank you for your review. I was not aware that sensor values can be translated via the srings.json. That is really a great concept.

I thought again about my PR. In my opinion, for the SMA integration my PR it is still the better way for the translation as of the following reasons:

  • The SMA integration is quite generic and suppports different devices with different sensors. Non numeric sensor values are not defined in the integration but directly in the device. All devices support language packages that are normaly pre-installed for the country where the device is installed. The translations in the device are always up-to-date and complete.
  • Defining the values and their translations in the integrations strings.json would never be complete, as it would not cover all devices and all of their versions.
  • The current version of the SMA integration selects always the english translation (hardcoded) which seams to be pre-installed on all devices. Most users of SMA devices would have their prefered language installed on the device but can't use it.

If you still don't like the way of translating the sensor values this way for the SMA integration, I will just close the PR. I fully respect your decision.

Thanks a lot for your awesome work for the HA community. frimtec

frimtec avatar Mar 24 '23 07:03 frimtec

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. Thank you for your contributions.

github-actions[bot] avatar Jun 22 '23 14:06 github-actions[bot]

In my opinion, for the SMA integration my PR it is still the better way for the translation

We won't accept that, sorry. We don't want to rely on third party sources for this. We support many more language.

Above all, a user should not be burdened with configuring languages on integrations, as they configure the language in Home Assistant already.

If you still don't like the way of translating the sensor values this way for the SMA integration, I will just close the PR.

Alright, I guess that will be the only path in that case. Feel free to re-open when you'd like to implement the translation on our end.

../Frenck

frenck avatar Jun 22 '23 22:06 frenck