Skip to content

Fix explicit empty text defaults - #474

Merged
HeyItsGilbert merged 1 commit into
mainfrom
fix/empty-text-default
Sep 13, 2026
Merged

HeyItsGilbert merged 1 commit into
mainfrom
fix/empty-text-default

Conversation

@HeyItsGilbert

@HeyItsGilbert HeyItsGilbert commented Sep 13, 2026

Copy link
Copy Markdown
Member

Summary

  • preserve an explicitly declared empty JSON default during manifest conversion
  • allow a text prompt to accept blank input only when its manifest explicitly declares a default
  • add end-to-end regression coverage for default: ""

Verification

  • pwsh -NoLogo -NoProfile -Command '$cfg = New-PesterConfiguration; $cfg.Run.Path = "./tests/EmptyDefaultTextParameter.Tests.ps1"; $cfg.Output.Verbosity = "Normal"; $cfg.Run.PassThru = $true; $result = Invoke-Pester -Configuration $cfg; exit $result.FailedCount'
  • 3 passed, 0 failed

Fixes #473.

Copilot AI lite review requested due to automatic review settings September 13, 2026 00:11
@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown

Test Results

    4 files  292 suites   1m 10s ⏱️
  251 tests 249 ✅ 2 💤 0 ❌
1 004 runs  996 ✅ 8 💤 0 ❌

Results for commit 22a008f.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Two moderate conversion issues remain, and the issue reference does not match the implemented scope.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR preserves explicitly empty text defaults during manifest conversion and allows blank input when such defaults are declared.

Changes:

  • Preserve empty JSON defaults.
  • Allow configured blank text responses.
  • Add regression and end-to-end coverage.
File summaries
File Review summary
tests/EmptyDefaultTextParameter.Tests.ps1 Adds regression coverage; the Fixes #43 reference appears unrelated to the implemented scope.
Plaster/Private/Resolve-ProcessParameter.ps1 Enables empty input for declared defaults; the linked issue’s requested localized-variable support is not implemented.
Plaster/Private/Read-PromptForInput.ps1 Implements optional blank input handling.
Plaster/Private/ConvertFrom-JsonManifest.ps1 Preserves empty defaults, but conditional processing and reverse conversion still require fixes to retain explicit empty defaults.
Review details

Suppressed comments (3)

Plaster/Private/ConvertFrom-JsonManifest.ps1:74

  • When a JSON text parameter has condition evaluate false and default is "", this conversion now emits the attribute, but Resolve-ProcessParameter still checks -not [string]::IsNullOrEmpty($default) and stores $null instead of the explicit empty string. That loses the newly supported default before content processing; use attribute presence in that branch and add a conditional regression test.
                    if ($param.PSObject.Properties.Match('default').Count -gt 0) {

Plaster/Private/ConvertFrom-JsonManifest.ps1:74

  • Please also update the reverse conversion. ConvertTo-JsonManifest still uses if ($paramNode.default) and therefore drops an XML default=""; an XML→JSON→XML round trip loses the explicit marker this change relies on, so the converted template stops accepting blank input. Add a HasAttribute('default') check there and a round-trip regression.
                    if ($param.PSObject.Properties.Match('default').Count -gt 0) {

Plaster/Private/Resolve-ProcessParameter.ps1:79

  • The linked issue #43 requests support for using localized-data hashtables (for example, $PLASTER_MyTemplate.ManifestComment) inside generated templates, but this change only preserves and accepts empty text defaults. As written, closing #43 from this PR does not satisfy that requirement; please correct the issue link/description or add the localized-variable implementation.
                $value = Read-PromptForInput $prompt $default $pattern -AllowEmpty:$Node.HasAttribute('default')
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/EmptyDefaultTextParameter.Tests.ps1
Preserve declared empty JSON defaults and distinguish them from missing defaults so blank text input is accepted only when explicitly configured.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused implementation correctly addresses the regression and includes comprehensive coverage.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@HeyItsGilbert
HeyItsGilbert merged commit b4345ec into main Sep 13, 2026
8 checks passed
@HeyItsGilbert
HeyItsGilbert deleted the fix/empty-text-default branch September 13, 2026 00:38
HeyItsGilbert added a commit that referenced this pull request Sep 13, 2026
## [2.1.4] - 2026-09-13

### Fixed

- Explicit empty text defaults in JSON manifests now remain empty rather
than being treated as absent, while blank prompt input is accepted only
when a default is declared
([#474](#474), closes
[#473](#473))
- Pester 6.1.0 compatibility for mocks and empty `-ForEach` data sets
([#475](#475))
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.

[Bug]: Optional field not allowing empty values.

2 participants