Add possibility of using an address list instead #202

DurgNomis-drol · 2021-12-01T16:08:38Z

This changes disable_discovery from a bool to address_list containing a dict. Example dict: ``{"Living room": "192.168.0.69"}` Not setting the address_list results in discovery been enabled. Setting it to and empty dict results in discovery being disabled.

I was not 100% sure on how to best implement this, so please provide feedback if something can be done better. 😄

Code is tested, but can someone double check to make sure that we are not breaking anything with this.

DurgNomis-drol · 2021-12-01T16:12:18Z

I am unsure of why the linter fails to install poetry

leikoilja

That's an amazing improvements, thanks man! Have a few suggestions

glocaltokens/client.py

tests/factory/providers.py

tests/test_client.py

DurgNomis-drol · 2021-12-02T18:26:45Z

Please see my changes and the last comment that are open 😃

leikoilja

Yay! Amazing job, man! Thanks 💯
Just a minor comment about default IPs/port, then we figure out why CI fails and it's good to go 🚀

p.s. speaking of CI and failing linters, can you please try few things:

rebase this branch to latest master when doing changes
run poetry install locally
run poetry run pre-commit install locally
run poetry run pre-commit run all-files locally
Hopefully it should help your linters get up to date and fix the issues :)

glocaltokens/client.py

tests/test_client.py

DurgNomis-drol · 2021-12-03T15:11:07Z

I have no idea of what went wrong and why it essentially deletes and the adds client.py and the new lines again, but i don't have time to fixed it today, will fixed it tomorrow

DurgNomis-drol · 2021-12-03T15:12:50Z

Can it be line endings?

DurgNomis-drol · 2021-12-04T15:49:58Z

Got it fixed, i have no idea what went wrong but rolled back to a working commit and fixed the linter.

@leikoilja Can I get you to release a new version after this is merged? Oh and can i get you to merged it? 😄

poetry.lock

KapJI · 2021-12-04T17:41:28Z

Can you give some context about what problem you're solving with this?

DurgNomis-drol · 2021-12-04T17:49:51Z

Can you give some context about what problem you're solving with this?

leikoilja/ha-google-home#340 is just one of them.

As i am working on moving the ha-google-home code to the separate module, i ran into some problems with zeroconf on WSL2. Code run in WSL2 can't discover devices through zeroconf. This just makes it much easier for me to test it while i am coding 😄

KapJI · 2021-12-04T18:16:37Z

Thanks, makes sense.

I think breaking compatibility for such change is not necessary, so let's keep disable_discovery.

Instead add extra_addresses argument in the end. IP address should be the key. And append it to list of discovered devices.

DurgNomis-drol · 2021-12-04T18:26:05Z

@KapJI We need to pass the name of the device as the key and the IP address as the value. If we don't, we can't match the IP address to the device and so on.

Making it a separate argument, means the user now needs to set disable_discovery to true and then set a dict with addresses. Using my way, we just disable it automatically if we have dict containing addresses.

KapJI · 2021-12-04T18:29:34Z

You don't need to disable discovery completely. If it crashes on WSL2, you can disable it automatically if running inside WSL2.

Mapping from IP to name makes more sense from UX perspective which I believe should come before implementation. You can build reverse dict here if needed.

DurgNomis-drol · 2021-12-04T18:35:07Z

I Think you are missing the point of what i am trying to do with this PR.

It dosen't make sense to reverse it like you are takling about. We match on device name not the IP.

KapJI · 2021-12-04T18:42:13Z

Ok, having name as a key is fine actually.

leikoilja · 2021-12-04T19:18:58Z

Nice!
Awesome thing we have more opinions and reviewers in here. I am not particularly worried about breaking compatibility, since we don't rely on that in ha-google-home and we can't really know if there is anyone using disable_discovery kwarg at all, so IMHO it shouldn't be a biggie. I ll leave it up to you, @DurgNomis-drol, to decide whether or not you want to refactor it into a separate kwarg keeping disable_discovery, but feel free to merge when you ready! 🚀
p.s. also given you the full repo permissions in the repo, not sure why it wasn't set in the first place 💯
Thanks man! 🔥

DurgNomis-drol · 2021-12-04T19:42:34Z

@leikoilja Thanks for both invites! Haven really been active here, so that is fine that i am only getting it now 😆

I will let @KapJI suggestions about disable_discovery tumble around in my head until tomorrow, just to make sure i am not missing something 😄 Right now i can't see the use for a separate argument

KapJI · 2021-12-05T09:23:22Z

Approach I proposes should be more flexible. Basically you can still use discovery and extend it with your own list. Also empty dict for disabling discovery while None enables it, is not very clear API.

DurgNomis-drol · 2021-12-06T14:28:46Z

@KapJI Good point about it not being clear the way i have proposed.

leikoilja

Amazing stuff, thanks, @DurgNomis-drol 🚀
To me, we are ready to merge 💥
@KapJI? :)

glocaltokens/client.py

Co-authored-by: Ilja Leiko <[email protected]>

Good catch! Co-authored-by: Ilja Leiko <[email protected]>

leikoilja · 2021-12-07T09:33:21Z

Amazing work, thank you, @DurgNomis-drol 💯 🔥

leikoilja · 2021-12-07T09:45:42Z

ping @DurgNomis-drol for v0.6 release

KapJI · 2021-12-07T10:56:51Z

glocaltokens/client.py

@@ -405,6 +429,14 @@ def find_device(unique_id: str) -> NetworkDevice | None:
                        unique_id,
                    )
                    network_device = find_device(unique_id)
+                elif item.device_name in address_dict:


This means if discovery is enabled, address_dict won't be used. That contradicts the idea to have separate disable_discovery flag.

Also if there's no item with such name in address_dict, it's worth to print a warning message.

KapJI · 2021-12-07T11:03:03Z

glocaltokens/client.py

@@ -335,6 +337,7 @@ def get_google_devices(
        self,
        models_list: list[str] | None = None,
        disable_discovery: bool = False,
+        addresses: dict[str, str] | None = None,


It's better to add new arguments in the end, to not break reverse compatibility.

leikoilja requested changes Dec 2, 2021

View reviewed changes

leikoilja assigned DurgNomis-drol Dec 2, 2021

leikoilja added the feature New feature or request label Dec 2, 2021

DurgNomis-drol requested a review from leikoilja December 2, 2021 18:26

leikoilja approved these changes Dec 3, 2021

View reviewed changes

glocaltokens/client.py Outdated Show resolved Hide resolved

glocaltokens/client.py Outdated Show resolved Hide resolved

tests/test_client.py Show resolved Hide resolved

DurgNomis-drol force-pushed the add-address-list branch from 3b3118d to c8d2fbf Compare December 4, 2021 15:39

KapJI reviewed Dec 4, 2021

View reviewed changes

poetry.lock Outdated Show resolved Hide resolved

DurgNomis-drol force-pushed the add-address-list branch from 79440fe to 08f7029 Compare December 4, 2021 17:44

Revert poetry update

9ef7340

DurgNomis-drol force-pushed the add-address-list branch from 08f7029 to 9ef7340 Compare December 6, 2021 14:34

DurgNomis-drol added 2 commits December 6, 2021 16:08

Re-add disable discovery

6f2d137

Don't change anything in scanner.py

dc58324

DurgNomis-drol requested a review from KapJI December 6, 2021 15:34

leikoilja reviewed Dec 6, 2021

View reviewed changes

glocaltokens/client.py Outdated Show resolved Hide resolved

glocaltokens/client.py Outdated Show resolved Hide resolved

glocaltokens/client.py Show resolved Hide resolved

glocaltokens/client.py Show resolved Hide resolved

DurgNomis-drol and others added 2 commits December 6, 2021 20:02

Update glocaltokens/client.py

c184d02

Co-authored-by: Ilja Leiko <[email protected]>

Update glocaltokens/client.py

20ef185

Good catch! Co-authored-by: Ilja Leiko <[email protected]>

leikoilja merged commit 81928d1 into leikoilja:master Dec 7, 2021

KapJI reviewed Dec 7, 2021

View reviewed changes

DurgNomis-drol deleted the add-address-list branch December 7, 2021 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility of using an address list instead #202

Add possibility of using an address list instead #202

DurgNomis-drol commented Dec 1, 2021

DurgNomis-drol commented Dec 1, 2021

leikoilja left a comment

DurgNomis-drol commented Dec 2, 2021

leikoilja left a comment •

edited

Loading

DurgNomis-drol commented Dec 3, 2021 •

edited

Loading

DurgNomis-drol commented Dec 3, 2021

DurgNomis-drol commented Dec 4, 2021 •

edited

Loading

KapJI commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021 •

edited

Loading

KapJI commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021 •

edited

Loading

KapJI commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021

KapJI commented Dec 4, 2021

leikoilja commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021

KapJI commented Dec 5, 2021 •

edited

Loading

DurgNomis-drol commented Dec 6, 2021

leikoilja left a comment

leikoilja commented Dec 7, 2021

leikoilja commented Dec 7, 2021

KapJI Dec 7, 2021

KapJI Dec 7, 2021

KapJI Dec 7, 2021

Add possibility of using an address list instead #202

Add possibility of using an address list instead #202

Conversation

DurgNomis-drol commented Dec 1, 2021

DurgNomis-drol commented Dec 1, 2021

leikoilja left a comment

Choose a reason for hiding this comment

DurgNomis-drol commented Dec 2, 2021

leikoilja left a comment • edited Loading

Choose a reason for hiding this comment

DurgNomis-drol commented Dec 3, 2021 • edited Loading

DurgNomis-drol commented Dec 3, 2021

DurgNomis-drol commented Dec 4, 2021 • edited Loading

KapJI commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021 • edited Loading

KapJI commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021 • edited Loading

KapJI commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021

KapJI commented Dec 4, 2021

leikoilja commented Dec 4, 2021

DurgNomis-drol commented Dec 4, 2021

KapJI commented Dec 5, 2021 • edited Loading

DurgNomis-drol commented Dec 6, 2021

leikoilja left a comment

Choose a reason for hiding this comment

leikoilja commented Dec 7, 2021

leikoilja commented Dec 7, 2021

KapJI Dec 7, 2021

Choose a reason for hiding this comment

KapJI Dec 7, 2021

Choose a reason for hiding this comment

KapJI Dec 7, 2021

Choose a reason for hiding this comment

leikoilja left a comment •

edited

Loading

DurgNomis-drol commented Dec 3, 2021 •

edited

Loading

DurgNomis-drol commented Dec 4, 2021 •

edited

Loading

DurgNomis-drol commented Dec 4, 2021 •

edited

Loading

DurgNomis-drol commented Dec 4, 2021 •

edited

Loading

KapJI commented Dec 5, 2021 •

edited

Loading