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 user name and integration name to sensor names and ids #120

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

ronlut
Copy link
Contributor

@ronlut ronlut commented Jan 10, 2024

image

We might remove the garmin_connect prefix in the entity id, it will simplify code and it might be unique enough without it. LMK

@ronlut ronlut mentioned this pull request Jan 10, 2024
@cyberjunky
Copy link
Owner

@ronlut Very nicely written PR, thank you! Will merge today!

@cyberjunky cyberjunky marked this pull request as ready for review January 16, 2024 12:25
@cyberjunky
Copy link
Owner

We might remove the garmin_connect prefix in the entity id, it will simplify code and it might be unique enough without it. LMK

I think we better have it in there, it's standard among other integrations as far as I can see, and easier to search for them in developer tools like states...

@cyberjunky cyberjunky merged commit a12306d into cyberjunky:main Jan 16, 2024
1 check passed
@ronlut
Copy link
Contributor Author

ronlut commented Jan 16, 2024

Thanks @cyberjunky, wasn't expecting you to merge it right away 😅
It's a breaking change (changes all ids basically) so it's worth alerting it somehow to users if you have a way...
Also, I tested it partially but some more tests would be good from your side (with your knowledge of the integration)

@cyberjunky
Copy link
Owner

I will make a clear statement in the changelog, I noticed that the gear sensors didn't have the entity_id yet, I added it locally, testing it now.

@cyberjunky
Copy link
Owner

Hmm not sure yet what to do, I have test accounts with + in email address, doesn't look so nice, replace special chars, or fetch fullname from garmin...

@ronlut
Copy link
Contributor Author

ronlut commented Jan 16, 2024

I thought about using the first/full name instead of email, it's possible if we want. Email accounts with plus sign is not a common use-case for garmin I believe, as it's a personal service... But anyway this is the actual username

@cyberjunky
Copy link
Owner

Getting the users's name from garmin requires an extra call to get user_summary.. hmm. maybe first part of email address is fine, maybe strip out any special characters, I have to look into renaming the device name as well, otherwise you see two 'Garmin Connect' devices, one for each account, as we are creating disturbance, we can better combine those.

@ronlut
Copy link
Contributor Author

ronlut commented Jan 17, 2024

@cyberjunky LMK how you would like to proceed, I am open to making any changes :)

I think two "devices" make sense, then you can handle each of them independently, I think I saw it usually implemented like that, for example:
https://github.com/dckiller51/bodymiscale

@cyberjunky
Copy link
Owner

@ronlut I'm still struggling on how to let people merge/convert their devices, I might pause this merge for some time until I find a good solutions, I have some links/ideas to check. I want to give priority to MFA functionality... let me think

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