Skip to content

Capture module prerelease in module information#213

Merged
PrzemyslawKlys merged 1 commit intomainfrom
codex/module-information-prerelease
Mar 13, 2026
Merged

Capture module prerelease in module information#213
PrzemyslawKlys merged 1 commit intomainfrom
codex/module-information-prerelease

Conversation

@PrzemyslawKlys
Copy link
Member

Summary

  • capture the module prerelease label on ModuleInformation
  • teach ModuleInformationReader to read prerelease from PrivateData.PSData.Prerelease with top-level fallback
  • add focused tests for both prerelease sources

Verification

  • dotnet build .\PowerForge.PowerShell\PowerForge.PowerShell.csproj -c Release -f net10.0 --no-restore
  • dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release -f net10.0 --filter "FullyQualifiedName~ModuleInformationReaderTests" --no-restore -p:BuildProjectReferences=false

Notes

  • the broader multi-target graph still has an unrelated net472 IsExternalInit failure in PublishVerificationHostService, so verification is scoped to the focused net10.0 path here.

@claude
Copy link

claude bot commented Mar 13, 2026

Code Review

This is a clean, well-scoped addition. The implementation is straightforward and the test coverage covers the two happy-path scenarios described in the PR. A few observations below.


Minor issues

1. Scalar field read via array API (ModuleInformationReader.cs:56-60)

PrivateData.PSData.Prerelease is always a scalar string in .psd1 manifests — the PowerShell Gallery spec defines it as a single label like 'beta1', never an array. Using TryGetPsDataStringArray for this field works (the method wraps a scalar string into a single-element array), but it is semantically misleading. A dedicated TryGetPsDataString would express intent more clearly, or at minimum a brief comment explaining why the array API is used here would help future readers.

// Using TryGetPsDataStringArray because no scalar-specific overload exists;
// Prerelease is always a single-value string in practice.

2. Missing test: both locations present (precedence)

Read_PrefersPrereleaseFromPsData only tests the PSData path in isolation. There is no test that sets both PrivateData.PSData.Prerelease and a top-level Prerelease, verifying that PSData wins. Without it, the else if branch could be silently broken — e.g. if the order were accidentally swapped — while both existing tests still pass.

3. Missing test: no prerelease present → null

There is no assertion that result.PreRelease is null when the manifest has no prerelease label. This is the default case for most modules and worth pinning explicitly.

4. Temp directory not cleaned up (ModuleInformationReaderTests.cs:49-58)

CreateModuleProject creates files under Path.GetTempPath() but never deletes them. Path.GetRandomFileName() avoids collisions between runs, so there is no correctness risk, but the leftover artefacts accumulate in CI. Implementing IDisposable on the test class (or wrapping in a using block) would keep CI agents tidy.

5. Empty-string prerelease not guarded

If a manifest has Prerelease = '' inside PSData, TryGetPsDataStringArray will return { "" }, the Length: > 0 check passes, and PreRelease is set to an empty string. The top-level TryGetTopLevelString path may handle empty values differently depending on its implementation. Worth adding a string.IsNullOrWhiteSpace guard or a test to document the intended behaviour.


What looks good

  • The priority order (PSData > top-level fallback) matches the PowerShell Gallery convention.
  • ModuleInformation follows the existing immutable constructor pattern correctly.
  • The property name PreRelease uses standard C# PascalCase while the manifest key Prerelease keeps its original casing internally — that separation is correct.
  • The XML doc comment on the new property is consistent with the surrounding docs.
  • Tests use raw string literals for manifest content which is readable and easy to maintain.

Since this is still a draft, these are all low-priority observations rather than blockers. The core logic is sound.

@PrzemyslawKlys PrzemyslawKlys marked this pull request as ready for review March 13, 2026 08:54
@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@PrzemyslawKlys PrzemyslawKlys merged commit 0afebc1 into main Mar 13, 2026
6 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the codex/module-information-prerelease branch March 13, 2026 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant