Conversation
Add IRUST_INTERNAL_STDERR_START marker to separate stdout and stderr. Display stderr in red when present. Remove trailing newline handling.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the bare REPL functionality to improve output handling and error reporting. The changes consolidate stdout and stderr processing, enhance error messages with actual stderr content, and reorganize the test structure.
- Refactored output handling to combine stdout and stderr in a single processing pipeline
- Enhanced error messages to include actual stderr content instead of generic messages
- Reorganized test file structure by moving command initialization and adding permission checks
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/bare_repl.test.ts | Reorganized test structure, moved command initialization, and added TERM permission check |
| crates/irust_repl/src/utils.rs | Enhanced stdout_and_stderr function to combine outputs and handle void cases |
| crates/irust_repl/src/lib.rs | Removed redundant trailing newline removal logic |
| crates/irust/src/irust/format.rs | Added stderr handling logic to split and format combined output |
| crates/irust/src/irust/art.rs | Improved error message to include actual stderr content |
| } | ||
|
|
||
| String::from_utf8(out).unwrap_or_default() | ||
| String::from_utf8_lossy(&combined).to_string() |
There was a problem hiding this comment.
The call to .to_string() after from_utf8_lossy is redundant since from_utf8_lossy already returns a Cow<str> that can be converted to String more efficiently using .into_owned() when needed, or the function could return Cow<str> directly.
| String::from_utf8_lossy(&combined).to_string() | |
| String::from_utf8_lossy(&combined).into_owned() |
| let (stdout, stderr) = output.split_once("IRUST_INTERNAL_STDERR_START").unwrap(); | ||
| let mut eval_output = PrintQueue::default(); | ||
| eval_output.push(PrinterItem::String(prompt, options.out_color)); | ||
| eval_output.push(PrinterItem::String(stdout.into(), options.eval_color)); | ||
| eval_output.add_new_line(1); | ||
| eval_output.push(PrinterItem::String(format!("Err: {stderr}"), Color::Red)); | ||
| eval_output.add_new_line(new_lines_after_output); | ||
| return Some(eval_output); |
There was a problem hiding this comment.
Using unwrap() after split_once can cause a panic if the string doesn't contain the expected delimiter. This is risky since the condition on line 110 only checks if the string contains the delimiter, but split_once could still fail in edge cases. Consider using proper error handling or a more robust parsing approach.
| let (stdout, stderr) = output.split_once("IRUST_INTERNAL_STDERR_START").unwrap(); | |
| let mut eval_output = PrintQueue::default(); | |
| eval_output.push(PrinterItem::String(prompt, options.out_color)); | |
| eval_output.push(PrinterItem::String(stdout.into(), options.eval_color)); | |
| eval_output.add_new_line(1); | |
| eval_output.push(PrinterItem::String(format!("Err: {stderr}"), Color::Red)); | |
| eval_output.add_new_line(new_lines_after_output); | |
| return Some(eval_output); | |
| if let Some((stdout, stderr)) = output.split_once("IRUST_INTERNAL_STDERR_START") { | |
| let mut eval_output = PrintQueue::default(); | |
| eval_output.push(PrinterItem::String(prompt, options.out_color)); | |
| eval_output.push(PrinterItem::String(stdout.into(), options.eval_color)); | |
| eval_output.add_new_line(1); | |
| eval_output.push(PrinterItem::String(format!("Err: {stderr}"), Color::Red)); | |
| eval_output.add_new_line(new_lines_after_output); | |
| return Some(eval_output); | |
| } else { | |
| // Fallback: treat the whole output as stdout, no stderr | |
| let mut eval_output = PrintQueue::default(); | |
| eval_output.push(PrinterItem::String(prompt, options.out_color)); | |
| eval_output.push(PrinterItem::String(output, options.eval_color)); | |
| eval_output.add_new_line(new_lines_after_output); | |
| return Some(eval_output); | |
| } |
No description provided.