refactor: extract comment removal service

[PrzemyslawKlys]

Summary

• move the Remove-Comments AST/token processing into a reusable PowerForge.PowerShell service
• keep the cmdlet focused on file/content input resolution, destination output, and PowerShell-facing behavior
• add focused tests for pre-param comments, explicit pre-param removal, and signature block preservation

Validation

• dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "CommentRemovalServiceTests|GitHubReleaseAssetWorkflowServiceTests|BuildConfigurationFactoryTests"
• pwsh .\Module\Build\Build-Module.ps1 -NoSign

[claude]

PR Review: Extract Comment Removal Service - Overall this is a clean refactor. See full review details below.

[claude]

Full review follows.

Bug fix baked in (good)

The original RemoveCommentsCommand checked for the param keyword twice in the token loop (once to reset doNotRemove, and again to set doNotRemoveCommentParam). The new service merges these into a single block with a continue, which is cleaner and correct. Worth noting in the PR description so reviewers understand it is intentional.

skipCommentsUntilParam check order is subtle

The continue inside the param branch means the skipCommentsUntilParam guard is never reached for the param token itself. That is correct, but a brief inline comment would help future readers understand the ordering is deliberate.

Redundant GroupBy overhead

GroupBy(token => token.Extent.StartLineNumber) was carried over from the original. The parser already returns tokens in document order, so the grouping adds LINQ overhead (one group object and a ToArray call per line) without any behavioral benefit. The inner for loop also only ever uses i to index lineTokens[i], so a flat foreach over tokens would be simpler. Pre-existing pattern, no blocker, but a good cleanup opportunity since the code is being moved anyway.

RemoveEmptyLines and RemoveAllEmptyLines interaction

When both switches are set, RemoveEmptyLines normalises line endings to CRLF first, then RemoveAllEmptyLines strips blank lines. The interaction is pre-existing behavior, but a comment clarifying the precedence rule would prevent future confusion.

Test coverage gaps

The three new tests cover the main scenarios. Suggested additions: (1) Process(null) throwing ArgumentNullException to validate the new null guard. (2) Empty or whitespace-only content to ensure Trim and the regex paths do not throw. (3) RemoveCommentsInParamBlock set to true, since the param-block depth-tracking logic is the most complex path and has no test yet. (4) RemoveEmptyLines and RemoveAllEmptyLines switches, since neither regex branch is exercised. (5) Script-level code with no function keyword to cover the scriptParamOffset equal to -1 branch.

Minor: CommentRemovalRequest init-only setters

Since the project targets LangVersion latest, properties could use init instead of set to express that the object is configured once at creation. Current mutable setters work fine. Style-only suggestion.

Minor: per-invocation CommentRemovalService allocation

The service is fully stateless. Instantiating it inline is harmless for single calls, but in a pipeline loop over many files it allocates a new object per record. A static instance or static method would avoid this. Low priority.

Summary

Architecture and separation of concerns: Clean improvement. Logic correctness: Faithful to the original with the double-param-check fix included. Test coverage: Core cases covered; param-block and empty-line paths missing. Performance: Minor redundant GroupBy and per-invocation allocation. Style: Consistent with project conventions.

Nice work. The cmdlet is much easier to read now.