-
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
chore: run pyupgrade #968
base: master
Are you sure you want to change the base?
chore: run pyupgrade #968
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 c7d1769 in 28 seconds
More details
- Looked at
434
lines of code in20
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. python/composio/server/api.py:262
- Draft comment:
The change fromstdout=subprocess.PIPE, stderr=subprocess.PIPE
tocapture_output=True
is appropriate and aligns with modern Python practices. This change is also applied inpython/composio/tools/local/anthropic_computer_use/actions/computer.py
andpython/composio/tools/env/host/shell.py
. - Reason this comment was not posted:
Confidence changes required:0%
The use ofcapture_output=True
in subprocess.run is a more concise and modern approach introduced in Python 3.7, replacing the older method of separately specifyingstdout=subprocess.PIPE
andstderr=subprocess.PIPE
. This change is appropriate and aligns with the intent of the PR to modernize the code for Python 3.8+.
2. python/composio/cli/add.py:289
- Draft comment:
The change from using a list comprehension to a set comprehension is appropriate and improves performance by directly creating a set. This change is also applied inpython/composio/tools/env/filemanager/file.py
. - Reason this comment was not posted:
Confidence changes required:0%
The change from using a list comprehension to a set comprehension is appropriate here. It simplifies the code and improves performance by directly creating a set, which is the desired data structure. This change is also applied inpython/composio/tools/env/filemanager/file.py
.
Workflow ID: wflow_7EekfqhVPTvtkIEN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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: |
@@ -210,12 +209,12 @@ def setup(self) -> None: | |||
def _send(self, buffer: str, stdin: t.Optional[str] = None) -> None: | |||
"""Send buffer to shell.""" | |||
if stdin is None: | |||
self.channel.sendall(f"{buffer}\n".encode("utf-8")) | |||
self.channel.sendall(f"{buffer}\n".encode()) |
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.
While removing utf-8
from .encode()
is generally safe as it's the default encoding in Python 3, in network communication contexts (like SSH) it's often better to be explicit about encodings to prevent potential issues with different system locales or SSH implementations. Consider keeping the explicit encode('utf-8')
for network-related operations.
Code Review SummaryOverall, the changes look good and improve code modernization. Here's a detailed assessment: Positive Changes✅ Modernized Python syntax using Areas for Improvement
Risk Assessment🟢 Low Risk: Most changes are syntactic improvements VerdictThe PR is generally good and improves code quality. The changes are well-structured and consistent. Recommend merging after addressing the comments about encoding in network operations and exception handling specificity. Rating: 8/10 - Good improvements with minor suggestions for robustness. |
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 ee75893 in 14 seconds
More details
- Looked at
37
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. python/composio/tools/env/docker/scripts/__init__.py:124
- Draft comment:
The change in indentation for theValueError
message is unnecessary and should be reverted to maintain consistency with the rest of the codebase. - Reason this comment was not posted:
Confidence changes required:50%
The change in indentation for the ValueError message inget_shell_env
function is unnecessary and does not improve readability. It should be reverted to maintain consistency with the rest of the codebase.
Workflow ID: wflow_3enKyOvrvwEJd2Ci
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Ran
pyupgrade ./python/composio --py38-plus
Important
Modernize Python codebase to Python 3.8+ using
pyupgrade
, updating syntax and simplifying code across multiple files.set([...])
with{...}
inadd.py
,file.py
, andlsp_helper.py
.super(ClassName, self)
withsuper()
incatch_all_exceptions.py
.subprocess.PIPE
withcapture_output=True
inapi.py
,shell.py
, andcomputer.py
.encoding="utf-8"
fromopen()
calls where default is sufficient ingrep_utils.py
,queries/__init__.py
, andrepomap.py
.IOError
withOSError
ingit_repo_tree.py
andgrep.py
.abs.py
andlsp_helper.py
.This description was created by for ee75893. It will automatically update as commits are pushed.