Add disable key file parameter

[thecode]

PR #28 added option to skip using key file and store keys externally. However it still use the key file for reading & writing keys, see comment: https://github.com/bendavid/aiopylgtv/pull/28#issuecomment-754561997 This PR fixes it by adding a parameter which allow complete disable of reading & writing keys to the SqliteDict key file. Change is important to allow Home Assistant store key per device and update IP in case it is changed.

[chros73]

I don't think this is necessary:

• if you specified a client key properly then it won't read anything from db
• it will never writes the specified key into db, and it will only write a new key upon pairing if the specified key is invalid (otherwise it doesn't)

I guess the key wasn't passed properly into WebOsClient (you can't specify key via command line yet).

[thecode]

I don't think this is necessary:

• if you specified a client key properly then it won't read anything from db
• it will never writes the specified key into db, and it will only write a new key upon pairing if the specified key is invalid (otherwise it doesn't)

I guess the key wasn't passed properly into WebOsClient (you can't specify key via command line yet).

When pairing a new device the key is always written: https://github.com/bendavid/aiopylgtv/blob/6b676ea69cb77f4e363d8fc5934cc126d07ad537/aiopylgtv/webos_client.py#L213 When paring a new device you don't have a key so the client will try to read it from the file: https://github.com/bendavid/aiopylgtv/blob/6b676ea69cb77f4e363d8fc5934cc126d07ad537/aiopylgtv/webos_client.py#L113

The idea is to drop the use of this file, while I agree the file is harmless if we transition to config flow and still keep the file it just make something which is not clear why it is there, I have not seen any integration which stores files inside Home Assistant root config folder.

Anyhow, I noticed another missing implementation to finish my Home Assistant PR which is reading a unique id from the TV, I did not submit it since this PR sits here for two months already and last commit was 4 months ago. I hope the repo will still be maintained. I noticed you already cloned it for another usage.

[bendavid]

Yes it's still maintained, but slowly. I'll come back to this PR as possible.

[chros73]

When pairing a new device the key is always written: When paring a new device you don't have a key so the client will try to read it from the file:

Yes, that's correct.

The idea is to drop the use of this file, while I agree the file is harmless if we transition to config flow and still keep the file it just make something which is not clear why it is there, I have not seen any integration which stores files inside Home Assistant root config folder. Anyhow, I noticed another missing implementation to finish my Home Assistant PR which is reading a unique id from the TV...

Oh, so do you say that Home Assistant uses its own config instead of aiopylgtv sqlite db? (I have never used it.) If so, then that's why you want to disable sqlite db completely. But you can specify the sqlite db path to aiopylgtv, if that helps: https://github.com/bendavid/aiopylgtv/blob/711abda131203ea9dc3076736f96b3316af7a5bb/aiopylgtv/webos_client.py#L60

I noticed you already cloned it for another usage.

Yes, I also created couple of PRs so far, and I created another small project bscpylgtv as well, mostly for command line usage.

Yes it's still maintained, but slowly. I'll come back to this PR as possible.

Nice, I'll have more PRs, if you will have time, we can deal with them quickly.

[thecode]

Oh, so do you say that Home Assistant uses its own config instead of aiopylgtv sqlite db? (I have never used it.) If so, then that's why you want to disable sqlite db completely. But you can specify the sqlite db path to aiopylgtv, if that helps

Yes the idea is to support SSDP / mDNS (zerconf) discovery, which means we listen to broadcasts from the TV, can detect a new TV and the user only have to approve pairing to the TV.

The key, IP address and TV unique ID (which is something else I need to work on to support in the lib) is stored within Home Assistant together with other device related data. This will also allow working in DHCP environments where the TV IP address may change, Home Assistant already know to detect IP change and update the new IP. As I said the file can stay but it would be an unclean solution, especially in a case (not common one) that two TVs will switch IP with each other (may happen after power failure) and the file will hold incorrect keys. It will not be harmful but the file is not needed.

Btw, There is a great work done in development of this library, so I am not saying it is not designed correctly, but usually we separate device communication class from storage class. So a device library will only handle the communication to the device and let the user handle the storage. Some libraries provide a storage as an helper class if the user wants to use it but it is not tied to the communication class.

[chros73]

Yes the idea is to support SSDP / mDNS (zerconf) discovery, which means we listen to broadcasts from the TV, can detect a new TV and the user only have to approve pairing to the TV.

There's one issue with this when the other device is on a different subnet, so providing the ability to set IP manually is a must.

The key, IP address and TV unique ID (which is something else I need to work on to support in the lib) is stored within Home Assistant together with other device related data. This will also allow working in DHCP environments where the TV IP address may change, Home Assistant already know to detect IP change and update the new IP. As I said the file can stay but it would be an unclean solution, especially in a case (not common one) that two TVs will switch IP with each other (may happen after power failure) and the file will hold incorrect keys. It will not be harmful but the file is not needed.

Now I understand, thanks for the info.

Btw, There is a great work done in development of this library, so I am not saying it is not designed correctly, but usually we separate device communication class from storage class. So a device library will only handle the communication to the device and let the user handle the storage. Some libraries provide a storage as an helper class if the user wants to use it but it is not tied to the communication class.

Good idea, it can be done with interfaces and passing a storage class into the main class, if the storage class is not set then obviously that part of the code won't be triggered.

[chros73]

usually we separate device communication class from storage class

I've implemented this as well:

• use type.Protocol to enforce required methods (requires Python 3.8 (instead of 3.7))
• provide WebOsClient.get_storage() to access the used storage class (example)
• export the default StorageSqliteDict class in the module (example)
• obviously command line util is not supported (only via scripting)

@bendavid , if you will like this change then this PR won't be needed. And this is my last change, truly :)

[thecode]

I've implemented this as well:

• use type.Protocol to enforce required methods (requires Python 3.8 (instead of 3.7))
• provide WebOsClient.get_storage() to access the used storage class (example)
• export the default StorageSqliteDict class in the module (example)

@chros73 I like the StorageProto implementation which allows using your own storage. I would like to discuss with you about future implementations and consult for the best option to implement them. Is it possible to discuss them with you? I can be reached on discord thecode#6829 or if you have another preferred way let me know. Thanks.

[chros73]

@thecode, yes, sounds good, but don't have unrealistic expectations :) For starter, make a small PR here and provide a list of ideas and their justification in the comment and we can carry on from there.

[thecode]

@thecode, yes, sounds good, but don't have unrealistic expectations :) For starter, make a small PR here and provide a list of ideas and their justification in the comment and we can carry on from there.

Thanks, the PR itself is small (reading device unique ID), but I don't want to work on it without knowing if and when it can happen. I have 1200 lines PR which was already merged to a branch in Home Assistant few months ago just waited for changes in the lib, now getting back to it and remember where I stopped will need many efforts. If it is possible to discuss what is needed (not over GitHub issues) let's do it. If not I accept that as everyone is volunteering on his free time, but I don't want to spend more time on things that won't happen.

[chros73]

I have 1200 lines PR

:) My gosh :) Send me an email then (you see it in my package info and we can carry on from there).

[chros73]

@bendavid , we have implemented what @thecode wanted: hello info I don't plan anything else :)