feat: Adding sed command to rtk list#1511
Conversation
|
|
📊 Automated PR Analysis
SummaryAdds a new Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
There was a problem hiding this comment.
Pull request overview
Adds a new rtk sed system command to the CLI, intended to run native sed while compacting large stdout and preserving tracking/exit-code behavior.
Changes:
- Added a
Sedsubcommand to the Clap CLI and wired it intorun_cli()andis_operational_command(). - Introduced
src/cmds/system/sed_cmd.rsto executesedand compact stdout via head/tail retention + line truncation (with unit tests). - Updated user-facing documentation to list
rtk sedand its expected savings behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Registers rtk sed, dispatches execution, marks it operational, and adds an arg-parsing test. |
| src/cmds/system/sed_cmd.rs | Implements sed execution + stdout compaction and unit tests for the compaction behavior. |
| src/cmds/system/README.md | Documents the new system command module. |
| docs/guide/resources/what-rtk-covers.md | Adds sed to the coverage/savings table. |
| README.md | Adds rtk sed example usage to the command list. |
| let mut cmd = resolved_command("sed"); | ||
| for arg in args { | ||
| cmd.arg(arg); | ||
| } | ||
|
|
||
| let result = exec_capture(&mut cmd)?; |
There was a problem hiding this comment.
exec_capture forces stdin to Stdio::null(), so rtk sed will not work for common usages like echo foo | rtk sed 's/o/a/g' or rtk sed '...' (reading from stdin). Consider running sed via core::stream::run_streaming with StdinMode::Inherit (and ideally a streaming head/tail filter) so stdin is preserved and you don’t have to buffer unbounded stdout into memory for large outputs.
| @@ -0,0 +1,144 @@ | |||
| //! Runs sed and compacts large stdout while preserving native behavior. | |||
There was a problem hiding this comment.
This module-level doc comment says sed runs “while preserving native behavior”, but the implementation compacts/truncates stdout and (as written) disables stdin. Please reword to reflect what’s actually preserved (exit code, stderr) vs what changes (stdout compaction / potential stdin behavior).
| //! Runs sed and compacts large stdout while preserving native behavior. | |
| //! Runs sed, preserving the underlying exit code and stderr while compacting | |
| //! and truncating large stdout output. |
| let result = exec_capture(&mut cmd)?; | ||
| let filtered_stdout = compact_sed_output(&result.stdout); |
There was a problem hiding this comment.
exec_capture(&mut cmd)? returns a fairly generic error; please add anyhow::Context here (similar to other command modules) so failures are attributable to sed execution (e.g., missing binary, spawn failure).
Summary
rtk sedas a system command that runs nativesedunchanged and compacts large stdout.rtk sedin the command list and system command coverage docs.Test plan
cargo fmt --all && cargo clippy --all-targets && cargo testrtk <command>output inspected