1Password / connect-sdk-python

Python SDK for 1Password Connect
https://developer.1password.com/docs/connect
MIT License
200 stars 30 forks source link

Fix/timeout #102

Closed volodymyrZotov closed 5 months ago

volodymyrZotov commented 5 months ago

This PR fixes the issue when Client throws an error if OP_CONNECT_CLIENT_REQ_TIMEOUT env var is not set.

In addition get_timeout function is improved to cover the following cases:

Testing steps

The env var is set

  1. Up 1Password Connect server.
  2. Put this to main.py file
    
    from onepasswordconnectsdk.client import AsyncClient, Client
    from onepasswordconnectsdk import new_client

client: Client = new_client(host, token) #put your token an host vaults = client.get_vaults() print(vaults)

3. `export OP_CONNECT_CLIENT_REQ_TIMEOUT=30`
4. run `python main.py`

**Should use default timeout**
1. Up 1Password Connect server.
2. Put this to `main.py` file 

from onepasswordconnectsdk.client import AsyncClient, Client from onepasswordconnectsdk import new_client

client: Client = new_client(host, token) #put your token an host vaults = client.get_vaults() print(vaults)


3. run `python main.py`

Resolves #104 
codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.96%. Comparing base (627f59d) to head (4bcf4d7).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #102 +/- ## ========================================== + Coverage 76.60% 76.96% +0.36% ========================================== Files 27 27 Lines 1949 1980 +31 ========================================== + Hits 1493 1524 +31 Misses 456 456 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

raphapassini commented 5 months ago

Thanks for fixing this @volodymyrZotov, I've created a PR as well fixing this but you was faster than me. Sorry for the bug I've introduced :(

raphapassini commented 5 months ago

Fell free to close my PR in favor of yours.

volodymyrZotov commented 5 months ago

@raphapassini No worries! I also missed that when tested it. Thanks for the attempt to fix it! That looks like the right approach. But I lean toward merging this one as it also covers edge cases like setting None and non-numeric values.