fix(refacto-p1): unified cmds execution flow (+ rm dead code)#904
fix(refacto-p1): unified cmds execution flow (+ rm dead code)#904
Conversation
shared cmd exec flow and also removed some notes in technical docs (does not belong here)
c7db13d to
48ec28d
Compare
FlorianBruniaux
left a comment
There was a problem hiding this comment.
Good refactor overall. Single exit point is the right call, the exit_code_from_output helper is solid. One inconsistency survived the pass (see inline). All 1123 tests pass, 9/9 CI checks green. LGTM with minor fix.
| // Propagate original exit code | ||
| std::process::exit(exit_code) | ||
| if !output.status.success() { | ||
| return Ok(exit_code); |
There was a problem hiding this comment.
This exit_code was computed with .code().unwrap_or(1) at line 265, not with exit_code_from_output(). Signal kills on vitest return 1 instead of 128+signal, which breaks the zero-.code().unwrap_or(1) invariant the PR description claims.
One-line fix at line 265:
let exit_code = exit_code_from_output(&output, "vitest");
FlorianBruniaux
left a comment
There was a problem hiding this comment.
Good refactor. Single exit point is the right call and exit_code_from_output is solid. One thing that slipped through: vitest_cmd.rs line 265 still computes exit_code with .code().unwrap_or(1) instead of the new exit_code_from_output(&output, "vitest") helper. Signal kills on vitest will return 1 instead of 128+signal, breaking the zero-.code().unwrap_or(1) invariant the PR description claims. One-line fix, everything else looks good. All 1123 tests pass, 9/9 CI checks green.
|
Thanks for the review @FlorianBruniaux , missing migration have been addressed in last commit |
Part 2 of codebase refacto
Summary
never call process::exit() directly
exit_code_from_output()
Test plan