-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix: fetch default auth scheme from API during initiate_connection
#895
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
👍 Looks good to me! Reviewed everything up to 94ee762 in 18 seconds
More details
- Looked at
47
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. python/composio/tools/toolset.py:1356
- Draft comment:
Improved error message formatting for better readability and consistency. - Reason this comment was not posted:
Confidence changes required:0%
The error message in thefetch_expected_integration_params
function was updated to useapp.name!r
andauth_scheme!r
for better readability and consistency. This is a good change as it improves the clarity of the error message.
2. python/composio/tools/toolset.py:1387
- Draft comment:
Changedauth_scheme
default toNone
to allow dynamic fetching of default auth scheme from API. - Reason this comment was not posted:
Confidence changes required:0%
Theauth_scheme
parameter in theinitiate_connection
function was changed from a default value of"OAUTH2"
toNone
. This allows the function to dynamically fetch the default auth scheme from the API if not provided, which is a more flexible approach.
3. python/composio/tools/toolset.py:1401
- Draft comment:
Updatedauth_scheme
check to handleNone
values appropriately. - Reason this comment was not posted:
Confidence changes required:0%
The check forauth_scheme
ininitiate_connection
was updated to handleNone
values, which is necessary due to the change in default value. This ensures that the function behaves correctly whenauth_scheme
is not provided.
Workflow ID: wflow_B2UyytUUhPMkufKP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -1384,7 +1384,7 @@ def initiate_connection( | |||
redirect_url: t.Optional[str] = None, | |||
connected_account_params: t.Optional[t.Dict] = None, | |||
*, | |||
auth_scheme: AuthSchemeType = "OAUTH2", | |||
auth_scheme: t.Optional[AuthSchemeType] = None, |
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.
The docstring for initiate_connection
should be updated to reflect that auth_scheme
is now optional and will be fetched from the API if not provided. This helps maintain clear documentation for future developers.
Code Review SummaryOverall AssessmentThe changes look good and improve the authentication scheme handling by making it more flexible and user-friendly. The code is clean and the error messages are more informative. Strengths✅ Good error message improvements using repr() for better clarity Suggestions for Improvement
Testing Recommendations
The changes are well-structured and improve the codebase. With the suggested improvements implemented, this PR will be ready to merge. |
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
Important
initiate_connection
now fetches default auth scheme from API if not provided, with improved error message formatting intoolset.py
.initiate_connection
intoolset.py
now fetches defaultauth_scheme
from API if not provided.auth_scheme
changed from"OAUTH2"
toNone
.fetch_expected_integration_params
intoolset.py
.This description was created by for 94ee762. It will automatically update as commits are pushed.