-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
The run_ninja function has grown complex and would benefit from refactoring to improve readability and maintainability. Currently, it handles command construction, logging, and output streaming all within a single function, which increases cognitive load and makes testing more difficult.
Suggested Refactor:
Split run_ninja into three focused helper functions:
- Command-builder: Responsible for assembling the command arguments, working directory, job count, and build file.
- Command-logger: Handles redacting and logging the final command invocation.
- Output-streamer: Spawns threads to pump
stdoutandstderr, and returns when both finish.
This would allow run_ninja to act as a simple orchestrator, improving clarity and testability. Example structure:
pub fn run_ninja(
program: &Path,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
let mut cmd = build_ninja_command(program, cli, build_file, targets)?;
log_command(&cmd);
let mut child = cmd.spawn()?;
let (out_handle, err_handle) = stream_output(child.stdout.take().unwrap(), child.stderr.take().unwrap());
let status = child.wait()?;
out_handle.join().ok();
err_handle.join().ok();
if status.success() {
Ok(())
} else {
Err(io::Error::new(
io::ErrorKind::Other,
format!("ninja exited with {status}"),
))
}
}Each helper function would be self-contained and easily testable. See the original suggestion for example implementations.
Action Items:
- Refactor
run_ninjainto three helper functions as described above. - Ensure each helper is unit tested.
- Confirm that all existing functionality is preserved after refactoring.
I created this issue for @leynos from #152 (comment).
Tips and commands
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels