Skip to content

refactor: extract project version workflows#196

Merged
PrzemyslawKlys merged 1 commit intomainfrom
codex/thin-cmdlets-project-versioning
Mar 11, 2026
Merged

refactor: extract project version workflows#196
PrzemyslawKlys merged 1 commit intomainfrom
codex/thin-cmdlets-project-versioning

Conversation

@PrzemyslawKlys
Copy link
Member

Summary

  • move project version discovery and update logic into a reusable PowerForge service
  • thin Get-ProjectVersion and Set-ProjectVersion down to path resolution and PowerShell host behavior
  • move public project version models out of cmdlet files and refresh generated help output type names

Validation

  • dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "ProjectVersionServiceTests"
  • pwsh .\Module\Build\Build-Module.ps1 -NoSign

@PrzemyslawKlys PrzemyslawKlys added enhancement New feature or request codex .NET Pull requests that update .NET code labels Mar 11, 2026
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d42bf148a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +339 to +341
var content = File.ReadAllText(manifestFile);
var match = Regex.Match(content, "ModuleVersion\\s*=\\s*['\\\"]?([\\d\\.]+)['\\\"]?", RegexOptions.IgnoreCase);
if (!match.Success)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Read only top-level ModuleVersion from manifests

Using a plain regex here captures the first textual ModuleVersion assignment anywhere in the file, not specifically the manifest's top-level key. In manifests where RequiredModules (or another nested hashtable) appears before the top-level ModuleVersion, discovery can return a dependency version instead of the module's own version, which then makes Set-ProjectVersion -VersionType ... compute and apply an incorrect target version.

Useful? React with 👍 / 👎.

@claude
Copy link

claude bot commented Mar 11, 2026

Code Review: PR 196 - refactor: extract project version workflows

Overall this is a well-structured refactor. Separating discovery/update logic into PowerForge.ProjectVersionService makes the cmdlets clean and the service independently testable. The new models are well-documented. A few issues worth addressing before merging:


Bug 1 - Dead code in UpdateVersionNumber (Revision case)

The method pads versionParts to 4 elements before the switch statement, then the switch checks the same condition again, making the inner branch unreachable dead code. The condition versionParts.Count < 4 inside the switch is always false so the Add("1") call never executes. The inner guard should be removed.


Bug 2 - IsExcludedPath substring match causes false positives

path.IndexOf(fragment, StringComparison.OrdinalIgnoreCase) means a fragment like "bin" matches /Users/robbin/Projects/... or combine.csproj. The default fragments "obj" and "bin" are particularly susceptible. Path-segment boundary matching is needed:

var segments = path.Split(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
return segments.Any(seg => seg.Equals(fragment, StringComparison.OrdinalIgnoreCase));

Performance 1 - Update() traverses the file system twice

Update() calls Discover() a second time (without the ModuleName filter) causing a full second directory walk. Collect per-file versions during the first FindCandidateFiles pass instead.


Performance 2 - Each .ps1 file is read twice

LooksLikeBuildScript reads the file, then TryGetVersionFromBuildScript reads it again. A combined helper returning (bool isBuildScript, string? version) in one read halves I/O.


Code Quality 1 - ApplyVersionUpdate hardcodes ModuleVersion alignment

Hardcodes 8 spaces and single quotes regardless of the original file formatting. Capturing original whitespace in the regex and back-referencing it avoids silent formatting changes on every update.


Code Quality 2 - Bare catch {} in EnumerateFiles

try/catch blocks around Directory.EnumerateFiles and Directory.EnumerateDirectories swallow all exceptions including UnauthorizedAccessException. Catch only expected I/O exception types and surface them via the verbose callback.


Test Coverage Gaps

Several scenarios have no coverage:

Scenario Why it matters
ExcludeFolders path-segment exclusion Validates correctness, guards false-positive regression
ModuleName filtering Core filtering feature, currently untested
Explicit NewVersion string via Update() Second main code path for Set-ProjectVersion
Major, Build, Revision increment kinds Only Minor is exercised
No matching files raises InvalidOperationException Guards the documented error path

Summary: The architectural direction is clean and testability improves significantly. The two bugs (unreachable Revision branch, false-positives in IsExcludedPath) and the double file-system traversal in Update() are the highest priority fixes before merge. The ModuleVersion spacing issue and silent exception swallowing are lower-priority but worth addressing to avoid surprises in consumer repos.

@PrzemyslawKlys PrzemyslawKlys merged commit 4b39c94 into main Mar 11, 2026
8 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the codex/thin-cmdlets-project-versioning branch March 11, 2026 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex enhancement New feature or request .NET Pull requests that update .NET code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant