Normalize PowerShell casing across core APIs - #4281
Conversation
|
@claude review |
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Inactive
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Mechanical rename PR: Powershell* → PowerShell* casing across the core options records (PowerShellOptions, PowerShellScriptOptions, PowerShellFileOptions), the internal PowerShell implementation class, IPowerShellContext signatures, DI registration, tests, docs, and PublicAPI.Shipped/Unshipped.txt baselines.
Checked:
- Grepped the whole repo for leftover old-casing references (
Powershell). The only remaining hits are correctly left alone: Azure'sPowershellVersionand Chocolatey'sUseSystemPowershellare auto-generated from the external CLI tools' actual flag names, not part of this internal type rename, andInterfaceVisibilityTests.cs's"IPowershell"entry is an unrelated, already-removed legacy interface name being asserted absent (not this rename'sIPowerShellContext). PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtare updated consistently: old shipped members removed, new members added to Unshipped — the correct pattern for a breaking rename of already-shipped API, and it's documented as a breaking change inRELEASE_NOTES_V4.md.- New
PowerShellApiSurfaceTests.csasserts both that the new type names exist and that the old ones are gone from the assembly — a good regression guard against the casing drifting back or a partial rename slipping through. [ExcludeFromCodeCoverage]+ XML doc additions on thePowerShell*Optionsrecords now match the siblingBash*Optionsrecords, so the "align coverage exclusions" part of the PR description checks out.- File renames (
Powershell.cs→PowerShell.cs,PowershellTests.cs→PowerShellTests.cs) were done via git rename, preserving history.
No functional behavior changes, no missed call sites, no architectural concerns — this is a clean, well-scoped rename with good test coverage for the change itself.
Greptile SummaryThe PR consistently normalizes the core PowerShell API and implementation names to canonical casing.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines/Context/Domains/Shell/IPowerShellContext.cs | Updates option overloads to canonical type names and narrowly scopes the intentional overload warning suppression. |
| src/ModularPipelines/Context/PowerShell.cs | Renames the internal implementation and consistently constructs and accepts the canonical option types. |
| src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs | Updates the PowerShell context registration to resolve the renamed implementation. |
| src/ModularPipelines/Options/PowerShellOptions.cs | Introduces the canonically cased base PowerShell option record while preserving its command-line tool behavior. |
| src/ModularPipelines/Options/PowerShellScriptOptions.cs | Replaces the former script option record with the canonically named equivalent and unchanged CLI mapping. |
| src/ModularPipelines/Options/PowerShellFileOptions.cs | Renames the file option record while retaining its file argument and inheritance behavior. |
| src/ModularPipelines/PublicAPI.Shipped.txt | Removes the former spellings from the shipped API baseline as part of the intentional v4 breaking rename. |
| src/ModularPipelines/PublicAPI.Unshipped.txt | Records the complete canonical PowerShell API surface. |
| test/ModularPipelines.UnitTests/Api/PowerShellApiSurfaceTests.cs | Verifies that canonical API identities are exposed and legacy spellings are absent. |
Reviews (2): Last reviewed commit: "fix(api): scope PowerShell overload warn..." | Re-trigger Greptile
|
@claude review |
GitHub Actions
Failure
There was a problem hiding this comment.
Review
Re-reviewed after the new commit (de022235) since the prior CLEAR review at 8d48d738. The only change in that window is in src/ModularPipelines/Context/Domains/Shell/IPowerShellContext.cs:
- Adds
#pragma warning disable RS0026/restorearound the fourRunAsync/RunFileAsyncoverload pairs (string vs. options-record), suppressing the "ambiguous optional-parameter overload" analyzer warning for the intentionally-overlapping signatures.
Checked:
- This mirrors an existing, established pattern in
IModuleContext.cs(same rule, same disable/restore shape, same style of justification comment) for generic/non-generic delegate overloads that intentionally share an optional-cancellation-token shape — so this isn't a one-off suppression, it's consistent with how the codebase already handles this exact analyzer tradeoff. Directory.Build.propsonly excludesRS0026fromWarningsAsErrorsoutside CI (EnableCiAnalyzersunset), meaning it is an error under CI, which explains why the explicit pragma is needed here rather than relying on the local dev exemption.- The disable/restore pair is scoped tightly to just the four overloads that are actually ambiguous (
RunAsync(string, ...)/RunAsync(PowerShellScriptOptions, ...)andRunFileAsync(string, ...)/RunFileAsync(PowerShellFileOptions, ...)), not the whole file or a broader suppression — good scoping, avoids masking unrelated warnings in the same file. - No other files changed in this commit; the rest of the PR (the
Powershell*→PowerShell*rename) was already reviewed and is unchanged since the prior CLEAR verdict.
No functional or architectural concerns. Clean, well-scoped, consistent with existing conventions.
GitHub Actions
Inactive
1 participant
Summary
Powershell*option records and implementation to canonicalPowerShell*casingValidation
PowerShellApiSurfaceTests: 1/1 passedPowerShellTests: 4/4 passedCloses #4233