core icon indicating copy to clipboard operation
core copied to clipboard

Improvements for Habitica integration, adds icons and translations

Open tr4nt0r opened this issue 10 months ago • 12 comments

Breaking change

Entities didn't have unique id before, so the sensors will be re-created.

Proposed change

This PR gives the Habitica integration some TLC in preparation for migrating the sensors with tasks to the todo platform.

SensorEntityDescriptions were added to the normal sensors, as well as translation strings and icon translations. Also sensors now have a unique id.

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)
  • [x] 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:

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
  • [x] I have followed the perfect PR recommendations
  • [x] 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.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

tr4nt0r avatar Apr 26 '24 02:04 tr4nt0r

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

Code owner commands

Code owners of habitica 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 habitica 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.

home-assistant[bot] avatar Apr 26 '24 02:04 home-assistant[bot]

It looks good to me, but I'm a bit rusty with this code, so it's better to wait for @leikoilja to comment.

asmfreak avatar Apr 26 '24 04:04 asmfreak

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 Apr 26 '24 08:04 home-assistant[bot]

Should we also delete the old SENSOR_TYPE?

I really wanted to delete it, but then realized it is imported from init and used in the service schema but i didn't want to touch the service yet, so i left it in for now. I think i might just remove the service, as it seems pretty complicated to use and it kept throwing exceptions.

tr4nt0r avatar Apr 26 '24 14:04 tr4nt0r

Wait why is it used in the service?

joostlek avatar Apr 26 '24 14:04 joostlek

Wait why is it used in the service?

i don't know, didn't want to explore further, that's something i postponed for later xD

tr4nt0r avatar Apr 26 '24 14:04 tr4nt0r

Seems it was still in the schema but unused in the service and marked as deprecated, so i think it is save to remove SENSOR_TYPES

tr4nt0r avatar Apr 26 '24 15:04 tr4nt0r

@joostlek There are also these Task sensors, that i haven't touched yet, because I'm planning to migrate them to todo entities. Should I remove them for now or with the PR that introduces the todo platform?

tr4nt0r avatar Apr 26 '24 17:04 tr4nt0r

They should be deprecated for 6 months

joostlek avatar Apr 26 '24 17:04 joostlek

Oh, well, then maybe i should add the max. health sensor back. I removed it because 50 is a fixed value for maximum health so that sensor is quite useless. What do I have to do for deprecating sensor entities?

tr4nt0r avatar Apr 26 '24 17:04 tr4nt0r

@joostlek ok, i added deprecation warnings for the old task sensors, hope that does the job. :D Will be working now on the todo entities. Should i add them to this PR or in a new PR?

tr4nt0r avatar Apr 26 '24 21:04 tr4nt0r

The PR is now starting to do too much, let's try to scope it down to its original scope

yeah, also think so, the deprecation warnings should be in the same PR as the addition of the todo entities

tr4nt0r avatar Apr 27 '24 09:04 tr4nt0r