Update subprocess types and improve error handling in run.py #485
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request improves the reliability and maintainability of the Sequence diagram for run function with improved error handlingsequenceDiagram
participant User
participant run
participant subprocess.Popen
participant proc.stdout
participant proc.stderr
participant CommandError
User->>run: run(args, ...)
run->>subprocess.Popen: Popen(args, stdout=PIPE, stderr=PIPE, ...)
subprocess.Popen-->>run: proc
loop Read stdout lines
run->>proc.stdout: readlines()
proc.stdout-->>run: line
run->>run: Filter and join stdout lines
end
alt code != 0 and check_returncode
loop Read stderr lines
run->>proc.stderr: readlines()
proc.stderr-->>run: line
run->>run: Filter and join stderr lines
end
run->>CommandError: raise CommandError(output, returncode, cmd)
CommandError-->>User: Exception
else code == 0 or not check_returncode
run-->>User: output
end
Updated class diagram for CommandErrorclassDiagram
class CommandError {
+output: str
+returncode: int
+cmd: str | list[str]
+__init__(output: str, returncode: int, cmd: str | list[str])
}
note for CommandError "cmd is now explicitly converted to string"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #485 +/- ##
=======================================
Coverage 54.08% 54.09%
=======================================
Files 40 40
Lines 3635 3627 -8
Branches 793 793
=======================================
- Hits 1966 1962 -4
+ Misses 1318 1314 -4
Partials 351 351 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
io.TextIOWrapperto handle encoding and decoding directly when reading from stdout/stderr.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tony
force-pushed
the
subprocess-run-mypy
branch
from
3ba540f to
4dcd9db
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
io.TextIOWrapperto avoid manual encoding/decoding. - The error messages could be improved by including the command that failed.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tony
force-pushed
the
subprocess-run-mypy
branch
4 times, most recently
from
a933a65 to
f47cfd2
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
subprocess.rundirectly instead of wrapping it, as it provides similar functionality with a more standard interface. - The type annotation changes look good, but ensure they align with the actual types returned by the subprocess methods to avoid runtime errors.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tony
force-pushed
the
subprocess-run-mypy
branch
from
49750a7 to
aa03d5f
Compare
uv run ruff check --select ALL . --fix --unsafe-fixes --preview --show-fixes; uv run ruff format .
Fixed 6 errors:
- src/libvcs/_internal/subprocess.py:
3 × PYI061 (redundant-none-literal)
3 × UP045 (non-pep604-annotation-optional)
tony
force-pushed
the
subprocess-run-mypy
branch
from
9653f76 to
d4015ff
Compare
|
@sourcery-ai review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Technical Details
Type System Improvements:
stdout_linesandstderr_linesaslist[bytes]cmdargument type inCommandErrorby ensuring string conversionError Handling:
PIPE_NOT_SET_ERRconstant for consistent error messagingCode Quality:
filter(None, ...)with list comprehension for better readabilityall_outputto maintain consistent typesTesting
Impact
These changes improve code reliability and maintainability without changing the underlying behavior. The updates make the code more robust by:
Summary by Sourcery
Improve error handling and type annotations in the
runfunction. Standardize error messages using constants and ensure consistent type handling for outputs. No user-facing behavior changes are included.Bug Fixes:
RuntimeErrorcaused by unchecked file descriptors in therunfunction.all_outputto maintain consistent types across different code pathsChores:
runmodule using more concise imports from thetypingmodule.PIPE_NOT_SET_ERRfor consistent error messaging related to file descriptors.filter(None, ...)with list comprehension for improved readability.Summary by Sourcery
Improves error handling and type annotations in the
runfunction. Standardizes error messages using constants and ensures consistent type handling for outputs.Bug Fixes:
all_outputto maintain consistent types across different code paths.Enhancements:
runmodule using more concise imports from thetypingmodule.filter(None, ...)with list comprehension for improved readability.Chores:
PIPE_NOT_SET_ERRfor consistent error messaging related to file descriptors.