fix: don't use deprecated api in toolset #987
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e120e28 in 17 seconds
More details
- Looked at
36lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/tools/toolset.py:492
- Draft comment:
Changedexecuteto_executeto avoid using deprecated methods. This aligns with the intent to use non-deprecated methods internally. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_AEsX1RjEwSc8jlyj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
python/composio/client/__init__.py
Outdated
| @@ -251,6 +251,17 @@ def execute( | |||
| :param session_id: ID of the current workspace session | |||
| :return: Dictionary containing execution result | |||
| """ | |||
| self._execute(action, params, connected_account_id, session_id, text, auth) | |||
There was a problem hiding this comment.
The execute() method is not returning the result of _execute(). This could lead to unexpected behavior since callers expecting a return value will receive None. Please add a return statement:
def execute(self, ...):
return self._execute(action, params, connected_account_id, session_id, text, auth)
Code Review SummaryThe changes look generally good and move in the right direction by refactoring deprecated API usage. Here's a summary of the review: Positive Points✅ Good separation of concerns by moving core logic to private method Areas for Improvement
Overall AssessmentThe code quality is good (7/10) with minor improvements needed around documentation and return value handling. The refactoring approach is solid and follows good practices for managing deprecated functionality. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 43691a4 in 10 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Nj95CIiKij5sn8Tw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 76b009b in 36 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_4esfAOpCTGnzYK3S
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -489,7 +489,8 @@ def _execute_remote( | |||
| if auth is None: | |||
| self.check_connected_account(action=action) | |||
|
|
|||
| output = self.client.get_entity(id=entity_id).execute( | |||
| entity = self.client.get_entity(id=entity_id) | |||
| output = entity._execute( # pylint: disable=protected-access | |||
There was a problem hiding this comment.
Using a protected member _execute directly is not a best practice. Consider providing a public method in the Entity class to handle this functionality.
Entity.executeis marked as deprecated for the users and will only be used internally in the SDK.Important
Replace deprecated
Entity.executewith internal_executemethod intoolset.pyand updateEntityclass in__init__.py.Entity.executewithEntity._executein_execute_remote()intoolset.py._execute()method inEntityclass in__init__.pyto handle execution internally.Entity.executeas deprecated for external use in__init__.py.This description was created by
for 76b009b. It will automatically update as commits are pushed.