Refactor: split large PowerForge service files#90
Conversation
Pull Request Review: #90 - Refactor Large PowerForge Service FilesSummaryThis PR successfully refactors 6 large service classes into partial classes, improving code maintainability by splitting ~16,300 lines across 39 files. The refactoring is purely structural with no behavior changes. ✅ Code Quality & Best PracticesExcellent Structural Organization
Code Organization Highlights
✅ Potential Bugs & IssuesNo critical issues found. The refactoring is structurally sound:
✅ Performance ConsiderationsNo performance impact. Partial classes are a compile-time feature:
✅ Security ConcernsNo security issues introduced. The refactoring maintains:
✅ Test CoveragePer PR description: "Builds and tests passed locally (PowerForge.csproj Release + PowerForge.Tests)" Since this is a pure refactoring with no behavior changes:
📋 Code Review Checklist
🎯 Recommendation✅ APPROVE - This is a well-executed refactoring that significantly improves code maintainability without introducing any issues. Benefits
Notes
💬 Minor Observations (non-blocking)
Great work on this refactoring! 🎉 Review conducted with Claude Code |
There was a problem hiding this comment.
Pull request overview
Refactors several large PowerForge services by splitting them into smaller partial class files to keep each file manageable while preserving the existing service APIs and overall structure.
Changes:
- Split
ModulePipelineRunnerinto multiple focused partials (planning, dependencies, merge/tests, formatting, scripting/paths, etc.). - Split
DotNetPublishPipelineRunnerandDotNetRepositoryReleaseServiceinto partials grouped by responsibilities (plan/run/steps/failure, execute/signing/versioning, etc.). - Split
ArtefactBuilder,ManifestEditor, andModuleValidationServicehelpers into dedicated partial files.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 92 comments.
Show a summary per file
| File | Description |
|---|---|
| PowerForge/Services/ModuleValidationService.Utils.cs | Extracts helper methods/types used by module validation into a utility partial. |
| PowerForge/Services/ModulePipelineRunner.Utils.cs | Utility helpers for file ops and formatting segment merging used by the pipeline runner. |
| PowerForge/Services/ModulePipelineRunner.ScriptingAndPaths.cs | Script execution helpers and path resolution helpers moved into a partial. |
| PowerForge/Services/ModulePipelineRunner.RequiredModules.cs | RequiredModules resolution helpers isolated into a dedicated partial. |
| PowerForge/Services/ModulePipelineRunner.Plan.cs | Planning logic extracted into a Plan partial implementation. |
| PowerForge/Services/ModulePipelineRunner.MissingAnalysis.cs | Missing command/module analysis and dependency inference moved into a partial. |
| PowerForge/Services/ModulePipelineRunner.MergeAndTests.cs | Merge, placeholder replacement, import, and post-merge tests moved into a partial. |
| PowerForge/Services/ModulePipelineRunner.FormattingSigningDelivery.cs | Formatting/signing/delivery-related functions split into a partial. |
| PowerForge/Services/ModulePipelineRunner.FormattingAndConsistency.cs | Formatting and file consistency logging helpers split into a partial. |
| PowerForge/Services/ModulePipelineRunner.Dependencies.cs | Build dependency install/resolution logic moved into a dependencies partial. |
| PowerForge/Services/ManifestEditor.TopLevel.cs | Top-level manifest operations separated into a partial file. |
| PowerForge/Services/ManifestEditor.PsData.cs | PSData-specific manifest operations separated into a partial file. |
| PowerForge/Services/ManifestEditor.Helpers.cs | Shared AST parsing/manipulation helpers extracted into a helper partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.VersionAndPacking.cs | Version resolution and packing logic split into a partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.ValidationAndSort.cs | Publish preflight validation and publish ordering logic split into a partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.TypesAndArgs.cs | Internal helper types / arg escaping split into a partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.Signing.cs | Package signing and certificate helpers split into a partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.PathsAndFilters.cs | Path utilities and package filtering split into a partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.ExpectedAndZip.cs | Expected-version map helpers and release zip creation split into a partial. |
| PowerForge/Services/DotNetRepositoryReleaseService.Execute.cs | Main release workflow (Execute) split into its own partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.Types.cs | Runner internal types/exceptions moved into a partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.Steps.cs | Restore/Clean/Build/Publish step implementations split into a partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.SigningAndProcess.cs | Process execution and signing helpers split into a partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.Run.cs | Main Run orchestration split into a partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.PublishLayout.cs | Publish layout/cleanup/zip operations split into a partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.Plan.cs | Planning logic for publish pipeline split into a partial. |
| PowerForge/Services/DotNetPublishPipelineRunner.FailureAndManifests.cs | Failure reporting and manifest writing split into a partial. |
| PowerForge/Services/ArtefactBuilder.Packaging.cs | Packaging-related helpers split into a partial. |
| PowerForge/Services/ArtefactBuilder.LocatorAndSave.cs | Module location/download/save helpers split into a partial. |
| PowerForge/Services/ArtefactBuilder.FileOps.cs | File/zip/cleanup helpers split into a partial. |
| PowerForge/Services/ArtefactBuilder.Api.cs | Public build API and main packaging flows split into a partial. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (topHash.KeyValuePairs.Count > 0) | ||
| { | ||
| var first = topHash.KeyValuePairs[0]; | ||
| var line = content.Substring(first.Item1.Extent.StartOffset, first.Item1.Extent.EndOffset - first.Item1.Extent.StartOffset); |
There was a problem hiding this comment.
There is an unused local variable line in InsertKeyValue (computed but never referenced). This adds noise and can trigger warnings; it can be removed without changing behavior.
| var line = content.Substring(first.Item1.Extent.StartOffset, first.Item1.Extent.EndOffset - first.Item1.Extent.StartOffset); |
| private static string BuildFindInstalledModuleScript() | ||
| { | ||
| return EmbeddedScripts.Load("Scripts/ModuleLocator/Find-InstalledModule.ps1"); | ||
| } |
There was a problem hiding this comment.
The closing brace for BuildFindInstalledModuleScript is mis-indented compared to surrounding code, which makes the file harder to scan and is inconsistent with the project’s formatting style. Reformat this method to match the prevailing indentation.
| } | |
| } |
| foreach (var kvp in deps) | ||
| { | ||
| foreach (var dep in kvp.Value) | ||
| { | ||
| if (inDegree.ContainsKey(dep)) | ||
| inDegree[dep]++; | ||
| } | ||
| } |
There was a problem hiding this comment.
The topological sort for publish order is inverted: deps maps project -> referenced projects, but inDegree is incremented for the dependency (dep) instead of the dependent (kvp.Key). This will publish projects before their referenced projects (e.g., A referencing B yields order A then B). Compute indegree as the number of dependencies per project and/or build a reverse adjacency list (dependency -> dependents) so referenced projects are ordered first.
| foreach (var module in requiredModules ?? Array.Empty<string>()) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(module)) | ||
| continue; | ||
| if (approvedModules is not null && approvedModules.Contains(module)) |
There was a problem hiding this comment.
approvedModules.Contains(module) uses the collection's default comparer, which is case-sensitive for arrays/lists. Since module names are treated case-insensitively elsewhere (e.g., StringComparer.OrdinalIgnoreCase), this can fail to skip approved modules when casing differs (notably when approvedModules comes from plan.ApprovedModules). Consider normalizing to a HashSet<string>(..., OrdinalIgnoreCase) inside this method (or using a case-insensitive Contains).
| foreach (var module in requiredModules ?? Array.Empty<string>()) | |
| { | |
| if (string.IsNullOrWhiteSpace(module)) | |
| continue; | |
| if (approvedModules is not null && approvedModules.Contains(module)) | |
| HashSet<string>? approvedLookup = null; | |
| if (approvedModules is not null) | |
| approvedLookup = new HashSet<string>(approvedModules, StringComparer.OrdinalIgnoreCase); | |
| foreach (var module in requiredModules ?? Array.Empty<string>()) | |
| { | |
| if (string.IsNullOrWhiteSpace(module)) | |
| continue; | |
| if (approvedLookup is not null && approvedLookup.Contains(module)) |
| foreach (var line in SplitLines(result.StdOut)) | ||
| { | ||
| if (!line.StartsWith("PFMODLOC::FOUND::", StringComparison.Ordinal)) continue; | ||
| var parts = line.Split(new[] { "::" }, StringSplitOptions.None); | ||
| if (parts.Length < 4) continue; | ||
|
|
||
| var version = Decode(parts[2]); | ||
| var path = Decode(parts[3]); | ||
| if (string.IsNullOrWhiteSpace(path)) continue; | ||
|
|
||
| var full = Path.GetFullPath(path.Trim()); | ||
| if (!Directory.Exists(full)) continue; | ||
|
|
||
| var verText = string.IsNullOrWhiteSpace(version) ? null : version.Trim(); | ||
| return new InstalledModule(full, verText); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
|
|
||
| var synopsisPercent = Percent(synopsisCount, total); | ||
| var descriptionPercent = Percent(descriptionCount, total); | ||
| var examplesPercent = Percent(exampleCount, total); |
There was a problem hiding this comment.
This assignment to examplesPercent is useless, since its value is never read.
| if (latest is not null && Version.TryParse(project.NewVersion, out var target)) | ||
| { | ||
| if (latest >= target && !spec.SkipDuplicate) | ||
| return (false, $"Publish preflight failed: {project.ProjectName} version {target} already exists (latest {latest}). Use -SkipDuplicate to allow."); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (latest is not null && Version.TryParse(project.NewVersion, out var target)) | |
| { | |
| if (latest >= target && !spec.SkipDuplicate) | |
| return (false, $"Publish preflight failed: {project.ProjectName} version {target} already exists (latest {latest}). Use -SkipDuplicate to allow."); | |
| } | |
| if (latest is not null | |
| && Version.TryParse(project.NewVersion, out var target) | |
| && latest >= target | |
| && !spec.SkipDuplicate) | |
| return (false, $"Publish preflight failed: {project.ProjectName} version {target} already exists (latest {latest}). Use -SkipDuplicate to allow."); |
| if (ast is PipelineAst p) | ||
| { | ||
| if (p.PipelineElements.Count == 1 && p.PipelineElements[0] is CommandExpressionAst ce) return ce.Expression; | ||
| } | ||
| if (ast is StatementAst s) | ||
| { | ||
| // Statement with a pipeline expression as content | ||
| if (s is PipelineAst ps) return AsExpression(ps); |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (ast is PipelineAst p) | |
| { | |
| if (p.PipelineElements.Count == 1 && p.PipelineElements[0] is CommandExpressionAst ce) return ce.Expression; | |
| } | |
| if (ast is StatementAst s) | |
| { | |
| // Statement with a pipeline expression as content | |
| if (s is PipelineAst ps) return AsExpression(ps); | |
| if (ast is PipelineAst p && p.PipelineElements.Count == 1 && p.PipelineElements[0] is CommandExpressionAst ce) | |
| { | |
| return ce.Expression; | |
| } | |
| if (ast is StatementAst s && s is PipelineAst ps) | |
| { | |
| // Statement with a pipeline expression as content | |
| return AsExpression(ps); |
| if (ast is StatementAst s) | ||
| { | ||
| // Statement with a pipeline expression as content | ||
| if (s is PipelineAst ps) return AsExpression(ps); |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (ast is StatementAst s) | |
| { | |
| // Statement with a pipeline expression as content | |
| if (s is PipelineAst ps) return AsExpression(ps); | |
| if (ast is StatementAst s && s is PipelineAst ps) | |
| { | |
| // Statement with a pipeline expression as content | |
| return AsExpression(ps); |
| if (ManifestEditor.TryGetTopLevelString(manifestPath, "RootModule", out var root) && | ||
| !string.IsNullOrWhiteSpace(root)) | ||
| { | ||
| if (!File.Exists(Path.Combine(moduleRoot, root))) | ||
| issues.Add($"RootModule missing: {root}"); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
PrzemyslawKlys
force-pushed
the
chore/split-large-services
branch
from
f59779e to
0808e36
Compare
Code Review: PR #90 - Refactor: Split Large PowerForge Service FilesSummaryThis PR successfully refactors several very large service classes (3900+ LOC) into smaller, logically organized partial class files. This is a pure refactoring with no behavior changes, which improves maintainability without introducing risk. Overall Assessment: ✅ APPROVEDThis is an excellent refactoring that significantly improves code maintainability. The changes are well-executed and follow C# best practices for partial classes. Strengths 💪1. Excellent Organization & Naming
2. Proper Partial Class Usage
3. Zero Behavior Changes
4. Improved MaintainabilityBefore:
After: Split into 5-10 focused files per service (150-850 LOC each) This makes the code much easier to:
Code Quality Assessment✅ Best Practices Followed
✅ No Bugs or Issues Detected
Performance Considerations✅ No Performance Impact
Security Considerations✅ Security UnchangedReviewed file operations and security-sensitive code:
Test Coverageℹ️ Test StatusAccording to the PR description:
Recommendation: Ensure CI pipeline validates:
Minor Observations📝 Naming Convention NotesFile naming is very good. A few observations:
💡 Future Consideration (Not blocking)If any of these partial files grow beyond ~500-600 LOC in the future, consider further subdivision. Current sizes are all reasonable. Recommendations✅ Ready to Merge with these verifications:
📚 Documentation (Optional)Consider adding a brief comment at the top of each main class file listing the partial files, e.g.: /// <summary>
/// Creates packed/unpacked artefacts for a built module using typed configuration segments.
/// </summary>
/// <remarks>
/// This class is split across multiple files:
/// - ArtefactBuilder.Api.cs - Build API methods
/// - ArtefactBuilder.FileOps.cs - File operations
/// - ArtefactBuilder.Packaging.cs - Package creation
/// - ArtefactBuilder.LocatorAndSave.cs - Module locating/saving
/// </remarks>
public sealed partial class ArtefactBuilderThis helps developers quickly understand the class structure (but not required). ConclusionThis PR exemplifies excellent refactoring practice:
Strongly recommend merging once CI validates all tests pass. Great work, @PrzemyslawKlys! This will make the PowerForge services much easier to work with going forward. 🎉 Reviewed by: Claude Code |
Splits very large PowerForge service classes into multiple
partialfiles to keep individual files manageable (no behavior changes).Scope:
Notes: