Add website artifact export support#524
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
PR Review: Add website artifact export support Overall this is a clean addition. A few items worth discussing before merging. ISSUE 1 - Breaking change to default build behavior (Build/Build-Module.ps1 line 83): ISSUE 2 - Documentation enabled globally (Build/Build-Module.ps1 line 73): ISSUE 3 - Inconsistency in manifest (Build/Export-WebsiteArtifacts.ps1 lines 99-112): ISSUE 4 - No error handling for git command (Build/Export-WebsiteArtifacts.ps1 line 92): POSITIVES:
MINOR NIT: The description field in the manifest is hardcoded. If this becomes a template for other Evotec hub modules, reading it from the PSD1 Description field would be more maintainable. |
|
PR Review: Add website artifact export support Overall this is a clean, well-structured addition. A few items worth discussing before merging. Build/Build-Module.ps1 Potential breaking change: RefreshPSD1Only default flipped The previous value was hardcoded to true (fast/PSD1-only mode). The new expression evaluates to false when the env var is unset, meaning a full build now runs by default. Existing CI pipelines or scripts that relied on the fast mode will be affected without any change on their end. If the old default should be preserved for routine builds, consider inverting the logic so that a full build is the opt-in behavior. Documentation always enabled Enabling docs on every build is required for Export-WebsiteArtifacts.ps1 but may slow down routine local runs. A short comment explaining the reason would help future maintainers. The backward-compat guard for SyncExternalHelpToProjectRoot is a nice touch. Build/Export-WebsiteArtifacts.ps1 surfaces.examples inconsistency The script copies the Examples directory into the artifact output, but the manifest declares examples = false. If examples are genuinely included, the flag should be true. If the flag carries a different meaning (e.g. not rendered as a website surface), that distinction should be commented. [regex]::Escape() redundant with -SimpleMatch The -SimpleMatch switch tells Select-String to treat the pattern as a plain literal string, making [regex]::Escape() a no-op. Only one of them is needed. git rev-parse HEAD not guarded against native-exe failure ErrorActionPreference Stop does not apply to native executables. If git is absent or the directory is not a git repo, the commit variable silently receives error text. Check LASTEXITCODE after the call to guard against this. ModuleVersion.ToString() can throw if the key is absent If ModuleVersion is missing from the PSD1, calling .ToString() on a null value throws a NullReferenceException. The null fallback already used for a missing PSD1 file should be extended to cover this case. .gitignore Clean additions. Ignoring WebsiteArtifacts/ and en-US/ keeps generated outputs out of version control. Summary
The core functionality is sound and the overall approach is well thought-out. The RefreshPSD1Only default change is the most impactful item to verify intentionality on. |
Summary
Validation