refactor: change build strategy to merge#195
Conversation
danteay
left a comment
There was a problem hiding this comment.
LGTM -- solid refactor that replaces the concurrent multi-process mockery strategy with a single merged invocation. The ~270 net lines removed (concurrency plumbing, goroutine pool, semaphore, progress channels) are replaced by a much simpler merge-then-execute-once approach. CI checks pass.
A few non-blocking suggestions:
1. Potential temp file leak on createSingleTempConfig failure
In Exec(), defer m.cleanup() is declared after createSingleTempConfig() returns. If that function partially writes a temp file (it appends to m.tmpFiles before returning) and then fails, the defer is never registered. Consider moving defer m.cleanup() earlier in Exec(), e.g., right before the merge/create calls.
2. Redundant doneChan in runSingleInvocation
Both the spinner path (spin.Action(...).Run()) and the non-TTY path are synchronous, so <-doneChan is always a no-op by the time it's reached. The channel + goroutine pattern could be simplified by removing doneChan entirely and calling the work function directly.
3. Empty packages edge case
If all package configs lack a packages key (or it's not a map), mergeSingleConfig returns a config with no packages. Mockery will run with nothing to do. A warning or early return might be helpful.
4. Backward compatibility of --jobs-num removal
Worth verifying that no CI scripts or Makefiles in consuming repos pass --jobs-num / -j, as they would break with an "unknown flag" error.
Overall this is a clean improvement. Nice work.
danteay
left a comment
There was a problem hiding this comment.
Second review pass -- all four suggestions from the first review have been properly addressed:
- Temp file leak --
defer m.cleanup()is now placed beforecreateSingleTempConfig()with an explanatory comment. Correct. - Redundant doneChan -- Entire channel/goroutine pattern removed.
runSingleInvocationnow calls work synchronously in both TTY and non-TTY paths. Clean. - Empty packages edge case -- Explicit
hasPackagescheck with warning and early return added aftermergeSingleConfig. Correct. - --jobs-num backward compatibility -- Acknowledged as a coordination concern, not a code defect.
Code quality is solid:
mergeSingleConfigcorrectly deep-merges duplicate package definitions so interfaces from multiple configs are preserved.- The overall flow in
Exec()reads linearly and is easy to follow. - CI checks pass (lint + commit_lint).
Approving.
Description
Se mergean todos los pkg en uno solo para performance