-
Notifications
You must be signed in to change notification settings - Fork 107
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
tmux_cmd
: Modernize to use text=True
#560
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #560 +/- ##
==========================================
- Coverage 88.68% 88.65% -0.04%
==========================================
Files 36 36
Lines 3924 3922 -2
Branches 362 362
==========================================
- Hits 3480 3477 -3
Misses 307 307
- Partials 137 138 +1 ☔ View full report in Codecov by Sentry. |
Reviewer's Guide by SourceryThis pull request refactors the Sequence diagram for simplified tmux_cmd execution flowsequenceDiagram
participant C as Client Code
participant T as tmux_cmd
participant S as subprocess.run
C->>+T: Execute tmux command
T->>T: Convert args to strings
T->>+S: Run command with text=True
S-->>-T: Return stdout/stderr as text
T->>T: Process stdout/stderr
T-->>-C: Return processed results
Class diagram showing tmux_cmd changesclassDiagram
class TmuxCommandResponse {
+list[str] stdout
+list[str] stderr
+int returncode
+list[str] cmd
+__init__(args)
}
note for TmuxCommandResponse "Removed console_to_str usage
Now uses text=True in subprocess.run"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
10b03d7
to
1a5e69a
Compare
With `text=True` being used we can now wrap in `str()` for bytes to be handled as `str`.
@sourcery-ai review |
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.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the unused 'reraise' function as well since it's not called anywhere (as noted in the TODO comment). This would further help in cleaning up legacy code.
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.
More resilience for `tmux` command across systems. Ability to debug because escape sequences preserves the byte value for debugging.
8ac9fe6
to
2a4901d
Compare
tmux_cmd
: Remove reliance on console_to_str
tmux_cmd
: Modernize to use text=True
) ## py(deps) libtmux 0.40.1 -> 0.42.0 - tree: https://github.com/tmux-python/libtmux/tree/v0.42.0 - releases: https://github.com/tmux-python/libtmux/releases/tag/v0.42.0 - changelog: https://libtmux.git-pull.com/history.html#libtmux-0-42-0-2025-02-02 - PyPI: https://pypi.org/project/libtmux/0.42.0/ ## run_before_script: Remove reliance on `console_to_str()` See also: tmux-python/libtmux#560
This is a legacy function dating back to Python 2.x and was derived from pip.
See: #558
Changes
tmux_cmd
: Usetext=True
insubprocess.run
See also:
tmux_cmd
: Remove usage ofconsole_to_str()
This is a legacy function dating back to Python 2.x and was derived from pip.
text=True
makes this unneeded.tmux_cmd
: Remove usage ofstr_from_console()
Originally from #131. In combination with
text=True
used intmux_cmd
this is unneeded.compat: Remove
console_to_str()
andstr_from_console()
Unused.
Summary by Sourcery
Update the
tmux_cmd
function to use thetext=True
argument insubprocess.run
, simplifying the function by removing the dependency on legacy encoding functions, and removing now-unused encoding functions.Enhancements:
tmux_cmd
by removing the dependency on the legacyconsole_to_str
function and using built-in string conversion.Tests:
tmux_cmd
and ensure compatibility with Python 3.