8000 Fallback smart device alias to model when nickname is empty by nicosemp · Pull Request #1521 · python-kasa/python-kasa · GitHub
[go: up one dir, main page]

Skip to content

Fallback smart device alias to model when nickname is empty #1521

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nicosemp
Copy link
@nicosemp nicosemp commented Apr 4, 2025

Fallback smart device alias to model if nickname is empty.

This will apply to any device, not just the P304M smart strip. If we prefer to have this exception just for the P304M I can add and model == "P304M" in the elif on line 601.

The end result:
image

Notice that the name will be P304M P304M, however this can easily be edited in the UI:
image

Entity ids look great:
image

Let me know if this solution is good or you would like to tweak it :)

EDIT: I see tests need fixing, I'll wait on your decision between having this fallback globally or only for P304M before fixing them.

@nicosemp
Copy link
Author
nicosemp commented Apr 4, 2025

Link to the original comments on the home-assistant/core PR

Copy link
Member
@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

We should improve this by using device type based user-friendly strings, e.g., "Power strip" for DeviceType.Strip etc. to help users who may have many of such devices, making it hard to distinguish among them.

Maybe we should additionally use some device-specific information (Mac address? Device id?) as a part of this generic alias. As this is only used when the device has no name, we can be verbose and rather relaxed about this.

What do you think, @sdb9696 ?

@nicosemp nicosemp force-pushed the nicosemp/fallback-alias-for-p304m branch from 385a798 to 5036e7d Compare April 5, 2025 23:48
Copy link
codecov bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.66%. Comparing base (e21ab90) to head (4937672).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1521   +/-   ##
=======================================
  Coverage   92.66%   92.66%           
=======================================
  Files         150      150           
  Lines        9538     9541    +3     
  Branches      974      975    +1     
=======================================
+ Hits         8838     8841    +3     
  Misses        499      499           
  Partials      201      201           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nicosemp
Copy link
Author
nicosemp commented Apr 5, 2025

@rytilahti that sounds even better. I copied the DEVICE_TYPE_TO_PRODUCT_GROUP dict in a new DEVICE_TYPE_TO_LABEL one and used it in the extra elif. I guess having these strings hardcoded in english is fine because people can rename them and it's just for devices with no nickname.

The result with the P304M is the name becomes Power Strip P304M (easily editable in the UI) and the ids begin with power_strip_ which works great.

I believe adding the mac address or device id would not be ideal, as you would be stuck with long "ugly" ids.

Out of curiosity: what happens if a user adds another identical power strip, or two identical power plugs? What's added to the ids to keep them unique? I don't have duplicates to test this.

Copy link
Member
@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Apologies for taking the time to respond, I added a couple of comments but this is looking good with a couple of minor nits! 👍

@@ -598,6 +616,8 @@ def alias(self) -> str | None:
"""Returns the device alias or nickname."""
if self._info and (nickname := self._info.get("nickname")):
return base64.b64decode(nickname).decode()
elif label := DEVICE_TYPE_TO_LABEL.get(self._device_type):
return label
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return label
return f"Unnamed {label} ({self.model} {self.device_id})"

How about something like this? As this is just a fallback when there is no name set, I think it makes sense to give the user as much information about the device as possible.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds great thank you! I fixed the other nits too, take a look whenever you get a chance.

Copy link
Author

Choose a reason for hiding this comment

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

Actually wait, I did push this but I believe this is exactly what causes the problem I was trying to fix.
The while string, including the Unnamed part, will be used as the device id as well as the beginning of all the connected entities' ids. It's a pain to rename all the ids to something more meaningful. And not renaming the ids is annoying when working with manual templates.

Since the power strip is unnamable on the Tapo app, what I'm trying to achieve is an acceptable default id so we don't have to rename all the entity ids. Can we use a simpler string that would be fine as a default device/entity id?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, sorry, indeed... I'm starting to wonder if it would be better to fix this somehow inside the homeassistant integration instead when a None is encountered? For the cli tool, we can do a pretty-printing otherwise. Would that make sense to you? This will also allow adapting the naming scheme in case homeassistant changes its scheme at some point.

@nicosemp nicosemp force-pushed the nicosemp/fallback-alias-for-p304m branch from 5036e7d to 164e37a Compare June 11, 2025 17:04
@@ -90,7 +94,7 @@ async def test_device_type_no_update(discovery_mock, caplog: pytest.LogCaptureFi
assert dev.device_type is DeviceType.Plug
assert (
repr(dev)
== f"<DeviceType.Plug at {DISCOVERY_MOCK_IP} - None ({short_model}) - update() needed>"
== f"<DeviceType.Plug at {DISCOVERY_MOCK_IP} - Plug ({short_model} {disco_id}) ({short_model}) - update() needed>"
Copy link
Author

Choose a reason for hiding this comment

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

@rytilahti I removed the Unnamed part from your suggested string, but I still see that the returned string doesn't match with the test unless I add ({short_model}) again, so maybe it's not needed on line 620 of kasa/smart/smartdevice.py because it's appended at a later step anyway?

In kasa/smart/smartdevice.py, how about we just do return f"{label} ({self.device_id})"?

@rytilahti rytilahti added the bug Something isn't working label Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0