adding connection timeout in secs with default value 10 secs

[skoky]

• adding connection timeout when opening socket to the device as an optional value with default 10 secs. Relates to Add support for timeout parameter on device connection #171
• small code formatting. Sorry for that, my IDE insists on this
• please add or review python code / sample if we maybe need to describe it in more details

[mihai-dinculescu]

Thank you for this. It's a handy addition.

Regarding usability, I would prefer if this were an optional function call rather than a mandatory parameter.
A lower default value than the current isahc default might make sense (10 seconds sounds good to me).

Something like this should be a better user experience:

    let device = ApiClient::new(tapo_username, tapo_password)?
        .set_timeout(timeout_secs) // <- optional usage
        .p100(ip_address)
        .await?;

This will also need to be added to the Python wrapper. LMK if you want to do it or if you would like help.

[skoky]

ApiClient::new(tapo_username, tapo_password)?

Yes, you are right, its probably more clean usage as you describe. But its much more difficult to implement as the ApiClient does not store client directly but protocol. And this is immutable and private.This would need bigger re-factoring.

In the current ApiClient::new there is HttpClient builder where we can set the timeout. And the timeout_secs is an optional value. Not mandatory.

[skoky]

I updated one python example tapo_p100.py with shorter timeout than defualt 10secs. I used 3 secs. Its optional value and default value is 10 secs as you suggested

[mihai-dinculescu]

I went ahead and did the refactor. It was a pain indeed, as you've assumed it would be. But it was worth it, as it enabled me to get rid of a Result and stop storing the username and password in multiple structures.

After a rebase from the main, adding a set_timeout function to the ApiClient should be trivial.
I've ended up going with a default of 30 seconds for the timeout, as it's a more typical value than 10 seconds.

[skoky]

I have rebased to your latest main. I feel i broke it. Let me analyze this in more details

[skoky]

I updated the rust API. Not sure how we can test python code to make sure it works. Do you like way of Rust and Python API is designed now?

[mihai-dinculescu]

Nice. This is a lot more ergonomic.
Maybe it should be called with_timeout or just timeout instead of set_timeout, considering it follows the builder pattern that returns Self?

The Python wrapper has examples that can be run like this.

[skoky]

I followed you style of setting params. Let me know if it make sense

[mihai-dinculescu]

Shouldn't the Python wrapper follow the same behavior of the Rust API with the with_timeout instead of having timeout in the constructor?

[skoky]

I think that the timeout param has a default value and may not be used. Its a standard pattern in Python. But if you want to change it, feel free to do it.

[WhySoBad]

How is this pull request coming along? I would really like to see this feature as soon as possible.

Let me know if I can help somewhere by contributing to this issue

[mihai-dinculescu]

Sorry for the long delay; work got in the way.
I've mulled this over and decided to go with your suggestion. While I would like the Python API to remain as consistent as possible with the Rust one, I find it worthwhile to diverge in some cases to make it Pythonic.

[skoky]

@mihai-dinculescu thanks for merging. Can you please release new version with python as well? I wish to have it in my project. Thanks

[mihai-dinculescu]

It's been released.