refactor: share buffered logging and scaffold bootstrap#195
refactor: share buffered logging and scaffold bootstrap#195PrzemyslawKlys merged 1 commit intomainfrom
Conversation
|
Code Review - PR 195 - see inline analysis |
|
Overall this is a clean focused refactoring PR. The BufferingLogger/BufferingLogEntry types were nearly identical in both PSPublishModule and PowerForge.Cli, and the WriteLogTail/FormatDuration helpers were copy-pasted verbatim. Moving them into the shared PowerForge library is the right call. |
|
Issues to Consider: (1) Thin wrapper in Program.Helpers.RunAndParse.cs - WriteLogTail is now a one-liner delegating to BufferedLogSupportService with no logic of its own. Callers could reference BufferedLogSupportService directly; the wrapper adds indirection without value. (2) Semantics of Attempted=false when base path is missing - when the directory check fails, the result returns Succeeded=false, Attempted=false, but the service did attempt the check. Consider Attempted=true here to distinguish active failures from no-op skips, or rename to WasApplicable/WasSkipped. (3) BufferedLogger.Entries exposes a mutable List - callers can add/remove entries externally. Exposing as IReadOnlyList with an internal backing field would prevent accidental mutation. WriteTail already accepts IReadOnlyList so the API is ready. (4) ModuleBuildScaffoldBootstrapResult should use init-only setters - for an internal result type only ever set at construction, init-only setters (C# 9+) would make intent clearer and prevent mutation. |
|
Minor nits: (a) Redundant null-forgiving operator - context.BasePathForScaffold! after IsNullOrWhiteSpace has already ruled out null; the exclamation mark is technically correct but redundant. (b) Unconditional service instantiation - logSupport = new BufferedLogSupportService() in InvokeModuleBuildCommand is created unconditionally but only used in the error path; harmless since the service is trivial but worth noting. Test coverage gaps: BufferedLogSupportServiceTests has no test for the no-truncation path (total entries at or below maxEntries) or for null/empty entry lists. ModuleBuildScaffoldBootstrapServiceTests has no test for the no-op path (BasePathForScaffold null or empty, expected Succeeded=true Attempted=false) or for context=null (should throw ArgumentNullException). Overall: good refactor, suggestions are mostly polish. The Attempted semantics question is the most worth discussing before merge. |
Summary
Validation