-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Add support for async token getters to ToolboxTool #147
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
Conversation
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.
Can we add a test for this behavior?
Absolutely, I will add these in an upcoming PR for unit tests (#150 ) |
A best practice is to include tests for features when they merge. Unless we have extenuating circumstances, let's try to follow that. |
0bcceb4
to
0724e38
Compare
That's a good idea. Unit tests added. Thanks! |
d11b5d4
to
7038c1d
Compare
0724e38
to
081dcf4
Compare
Looks great! Do you mind adding an e2e test for this functionality? |
081dcf4
to
05c881a
Compare
cd1d6a2
to
d3e20e5
Compare
a65149c
to
c80fadc
Compare
* dep: Add pytest-cov package as a test dependency. * chore: Remove unused imports from sync_client.py * chore: Add unit tests for the tool and client classes * chore: Delint * chore: Delint * chore: Cover tool not found case * chore: Add toolbox tool unit test cases * chore: Add additional test cases to cover tool invocation and better docstring validation. * chore: Add test cases for sync and static bound parameter. * chore: Reorder tests in matching classes. This will improve maintainability. * feat: Add support for async token getters to ToolboxTool (#147) * feat: Add support for async token getters to ToolboxTool * chore: Improve variable names and docstring for more clarity * chore: Improve docstring * chore: Add unit test cases * chore: Add e2e test case * chore: Fix e2e test case
Previously we were only supporting sync token getters. Adding support for async getters could be beneficial performance-wise.