-
Notifications
You must be signed in to change notification settings - Fork 32
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
Async await support #62
Conversation
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
==========================================
+ Coverage 74.38% 76.97% +2.58%
==========================================
Files 22 28 +6
Lines 1667 1928 +261
==========================================
+ Hits 1240 1484 +244
- Misses 427 444 +17
☔ View full report in Codecov by Sentry. |
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
Looks good overall but can you update the readme with info about using async |
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
Signed-off-by: volodymyrZotov <volodymyr.zotov@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on working on this complex task. So far it looks solid.
I will say that it's a bit tricky to easily compare the two clients and see the key differences in the current state.
With that, I have a couple of comments / questions:
- Is it possible to do a benchmark and see if the async client if faster than the normal one? Asking to see if it does a significant advantage to the user.
- Would it be better to split the async and sync client into separate files? I feel like it would help readability (for this review at least) and maintainability. Same goes for splitting the tests for async and sync clients.
# Conflicts: # README.md # poetry.lock
# Conflicts: # src/onepasswordconnectsdk/client.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review: ➖
Not needed since the main part of this PR is to 'duplicate' the functionality for an async client. By checking the diffs between the two clients, I can confirm that they have the same functionality. Therefore, folks who will use the async client only need to tweak the code related to getting the async client. The rest will remain the same.
Also, the tests made for the async client should give us the confidence that the async client works as expected.
Code review: ✅
This looks good to be merged.
I've left some nitpick comments that shouldn't block this from being merged.
Resolves #58
This PR adds the ability to use the client library in async way.
To initialize an async client you can pass
is_async: bool
flag tonew_client(url: str, token: str, is_async: bool = False)
function.By default, NON-async client is initialized.