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 manufacturer to prefname. #352

Merged
merged 5 commits into from
Nov 7, 2024
Merged

Conversation

Lash-L
Copy link
Contributor

@Lash-L Lash-L commented Nov 2, 2024

What spawned my conversation with you was that I got a tile for tracking my dog, and I struggled to find it a bit with Bermuda since it didn't have a name attached. I think adding manufacturer name to the end of a prefname when we don't have a real name and we are just making one up is a good step to help users figure out where the device they want to add is.

image

Copy link
Owner

@agittins agittins left a comment

Choose a reason for hiding this comment

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

Nice idea, and might be worth expanding in future to some more tag and watch makers.

I think we could replace bermuda_ with the slugified manufacturers name, eg tile_aa_dd_ff_44_33_55 (Tile) since the bermuda part was only there to make it obvious where some random mac-named entity came from.

device.name
or device.local_name
or f"{DOMAIN}_{slugify(device.address)}{f" ({slugify(device.manufacturer)}) "
if device.manufacturer else ""}"
Copy link
Owner

Choose a reason for hiding this comment

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

This is a me thing not a you thing, but I am having very strong visceral reactions to this ternary! Can you re-write it as a classic if-then-else? I keep trying to parse it and just get frustrated before getting anywhere 😅 I can't tell if it does what I expect because the surrounding indentation and quoting and parens make the usually smelly ternary thoroughly opaque to me! It might be an adhd limited working-set-memory thing, but I just don't have the stack space in my own brain for it, sorry! I feel like beyond a single line it's an antipattern.

While there... what do you think about ditching the {DOMAIN}_ part and replacing that with the slugified manu name as well? eg: tile_44_55_11_22_33 which is probably more helpful for users. I only put the domain in there because I didn't want to create entries with just a bare mac address, so having the manufacturer name prefixed (when available) I think is a better user-experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah replacing domain is a good call.

I removed the ternary.

I also added the whole list of member uuids. Maybe in the future we could setup a github action to automatically check for updates like once a week?

Copy link
Owner

@agittins agittins left a comment

Choose a reason for hiding this comment

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

This looks great! Agreed re setting up a job in the github workflow as well.

The suggested change addresses an async issue that gets flagged on my dev system, and I think is the right way to solve it.

custom_components/bermuda/coordinator.py Outdated Show resolved Hide resolved
@agittins agittins force-pushed the prefname_improvements branch from 3e248cc to 04e2dcb Compare November 7, 2024 03:24
Copy link
Owner

@agittins agittins left a comment

Choose a reason for hiding this comment

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

Whoo!

@agittins agittins merged commit 2dd6b38 into agittins:main Nov 7, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants