-
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
feat: Support requesting connected_account
in custom action
#1090
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 85adf95 in 59 seconds
More details
- Looked at
75
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. python/composio/tools/base/runtime.py:314
- Draft comment:
Update the comment to useauth_param
instead ofauth_params
for consistency with the variable name change. - Reason this comment was not posted:
Confidence changes required:50%
The variableauth_params
was renamed toauth_param
in the PR, but the comment on line 303 still uses the old nameauth_params
. This should be updated for consistency.
2. python/composio/tools/base/runtime.py:328
- Draft comment:
Update the comment to useauth_param
instead ofauth_params
for consistency with the variable name change. - Reason this comment was not posted:
Confidence changes required:50%
The variableauth_params
was renamed toauth_param
in the PR, but the comment on line 303 still uses the old nameauth_params
. This should be updated for consistency.
3. python/composio/tools/base/runtime.py:392
- Draft comment:
Update the comment to useauth_param
instead ofauth_params
for consistency with the variable name change. - Reason this comment was not posted:
Confidence changes required:50%
The variableauth_params
was renamed toauth_param
in the PR, but the comment on line 303 still uses the old nameauth_params
. This should be updated for consistency.
Workflow ID: wflow_JcexS7TCrj0idFvj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -253,12 +253,23 @@ def _parse_docstring( | |||
return header, params, returns | |||
|
|||
|
|||
def _get_connected_account(app: str, entity_id: str) -> t.Optional[ConnectedAccountModel]: |
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.
Consider adding docstring to the new _get_connected_account
function to maintain consistency with codebase documentation standards. The docstring should describe parameters, return type, and possible exceptions.
return connected_account | ||
except ComposioClientError: | ||
return None | ||
|
||
def _get_auth_params(app: str, entity_id: str) -> t.Optional[t.Dict]: |
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.
Consider extracting the common code between _get_connected_account
and _get_auth_params
into a shared helper function since they share similar initialization and error handling patterns. This would reduce code duplication and make future maintenance easier.
kwargs["connected_account"] = ( | ||
_get_connected_account(app=app, entity_id=metadata["entity_id"]) or {} | ||
) | ||
if auth_param: |
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 condition if auth_params > 0:
was changed to if auth_param:
which is more appropriate since it's a boolean flag. However, consider adding a type hint for this variable to make the boolean type explicit.
Code Review SummaryOverall, the changes look well-structured and maintain good coding practices. The addition of connected account support is implemented cleanly with proper error handling. Strengths:
Suggestions for Improvement:
Code Quality Rating: 8/10The code is well-written and maintainable, with just a few minor improvements suggested for documentation and code organization. |
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! Incremental review on b1626af in 13 seconds
More details
- Looked at
23
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. python/composio/tools/base/runtime.py:265
- Draft comment:
Remove the unnecessary newline after the function definition for consistency with the rest of the code style. - Reason this comment was not posted:
Confidence changes required:10%
The function_get_connected_account
has an unnecessary newline after the function definition, which is inconsistent with the rest of the code style.
Workflow ID: wflow_AzXG7aIjbhTUU7ef
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
It can be used like this:
Important
Adds support for
connected_account
parameter in custom actions by modifyingexecute()
and_build_executable_from_args()
inruntime.py
.connected_account
parameter in custom actions added inexecute()
and_build_executable_from_args()
inruntime.py
._get_connected_account()
to fetch connected account details.ConnectedAccountModel
inruntime.py
.This description was created by for b1626af. It will automatically update as commits are pushed.