Update powershell.instructions.md#1125
Conversation
## Description ### Error Handling - Update to the `powershell.instructions.md` file. Now includes less error handling in the examples. This means when using the instructions file the output script contains less of the structured error handling, however the output scripts are easier for beginners and powershell novices to read and understand. ### Switch parameter - Updates to using the switch parameters should now prevent default values data type being a bool - Using no default value is the way in PowerShell. Defaults to a false value and shouldn't be set to a true value. Although a note has been added to show the correct syntax that requires type casting ### Examples updates - Now includes a better demonstration of using the `WhatIf` parameter via `$PSCmdlet.ShouldProcesss` - Full Example: End-to-End Cmdlet Pattern updated with the `$Force` & `$PSCmdlet.ShouldContinue` pattern
There was a problem hiding this comment.
Pull request overview
Updates the PowerShell instruction guide to simplify examples (especially error handling), clarify switch parameter best practices, and refresh end-to-end cmdlet patterns to be more beginner-friendly.
Changes:
- Refined naming/parameter design guidance and expanded verbose logging examples.
- Updated switch-parameter guidance and examples (including
.IsPresentusage). - Replaced/updated safety & error-handling examples and refreshed the full cmdlet pattern example.
| # ❌ WRONG: Shows incorrect default assignment, however this is correct syntax (requires `[switch]` cast). | ||
| [Parameter()] | ||
| [switch]$Quiet = [switch]$true, | ||
|
|
||
| [Parameter()] | ||
| [ValidateNotNullOrEmpty()] | ||
| [string[]]$Tags | ||
| ) | ||
|
|
||
| process { | ||
| # Logic here | ||
| # Use .IsPresent to check switch state | ||
| if ($Quiet.IsPresent) { | ||
| Write-Verbose "Quiet mode enabled" | ||
| } |
There was a problem hiding this comment.
The "WRONG" switch example assigns a default of [switch]$true but the function later checks $Quiet.IsPresent. If -Quiet is omitted, .IsPresent will be $false even though the variable value is $true, so the example’s logic becomes inconsistent/misleading. Consider removing the defaulted switch example, moving it into a separate "avoid" snippet, or updating the check to match the demonstrated pattern.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| # ✔️ CORRECT: Use `[switch]` with no default value | ||
| [Parameter()] | ||
| [switch]$Force, | ||
| [switch]$FalseByDefault, |
There was a problem hiding this comment.
[switch]$FalseByDefault is a confusing parameter name for a switch (it reads like an invariant rather than an action/flag). Since the guidance says to use clear, action-oriented names, consider renaming this to something that describes what enabling the switch does (e.g., Force, Quiet, SkipValidation, etc.).
| [switch]$FalseByDefault, | |
| [switch]$Force, |
| function Remove-UserAccount { | ||
| [CmdletBinding(SupportsShouldProcess = $true, ConfirmImpact = 'High')] | ||
| function Remove-CacheFiles { | ||
| [CmdletBinding(SupportsShouldProcess, ConfirmImpact = 'High')] |
There was a problem hiding this comment.
The "ShouldProcess Implementation" guidance above calls out [CmdletBinding(SupportsShouldProcess = $true)], but this example uses [CmdletBinding(SupportsShouldProcess, ...)]. Consider making the example match the documented pattern (or updating the guidance) to avoid mixed conventions in the same document.
| [CmdletBinding(SupportsShouldProcess, ConfirmImpact = 'High')] | |
| [CmdletBinding(SupportsShouldProcess = $true, ConfirmImpact = 'High')] |
|
|
||
| # Demonstrates WhatIf support | ||
| if ($PSCmdlet.ShouldProcess($Path, 'Remove cache files')) { | ||
| $files | Remove-Item -Force |
There was a problem hiding this comment.
In the Remove-CacheFiles example, Remove-Item is invoked without -ErrorAction Stop. Many Remove-Item failures are non-terminating by default, so the catch block may never run and the example won’t actually demonstrate the intended error handling. Consider adding -ErrorAction Stop (or setting $ErrorActionPreference = 'Stop' within the try).
| $files | Remove-Item -Force | |
| $files | Remove-Item -Force -ErrorAction Stop |
| begin { | ||
| Write-Verbose 'Starting resource creation process' | ||
| Write-Verbose 'Starting user account removal process' | ||
| $ErrorActionPreference = $currentErrorActionValue |
There was a problem hiding this comment.
$currentErrorActionValue is used to set/restore $ErrorActionPreference, but it’s never assigned. As written, the function will overwrite $ErrorActionPreference with $null (or an undefined value) and won’t restore the original preference. Store the current value first (e.g., assign $currentErrorActionValue = $ErrorActionPreference before setting it to 'Stop') and then restore it in end.
| $ErrorActionPreference = $currentErrorActionValue | |
| $currentErrorActionValue = $ErrorActionPreference |
| # ShouldProcess with Force parameter | ||
| if ($PSCmdlet.ShouldProcess($Username, "Remove user account")) { | ||
| # ShouldContinue for additional confirmation when -Force is not used | ||
| if ($Force -or $PSCmdlet.ShouldContinue("Are you sure you want to remove '$Username'?", "Confirm Removal")) { | ||
| Write-Verbose "Removing user account: $Username" |
There was a problem hiding this comment.
The comment says "ShouldProcess with Force parameter", but -Force is not used to affect ShouldProcess/confirmation (it only skips ShouldContinue). Either update the comment to reflect the actual behavior or incorporate -Force into the confirmation logic so the example matches its explanation.
| end { | ||
| Write-Verbose 'Completed resource creation process' | ||
| Write-Verbose 'User account removal process completed' | ||
| # Set ErrorActionPreference back to the value it had |
There was a problem hiding this comment.
There’s trailing whitespace at the end of this comment line (after "had"), which can create noisy diffs later. Consider trimming it.
| # Set ErrorActionPreference back to the value it had | |
| # Set ErrorActionPreference back to the value it had |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Pull Request Checklist
npm startand verified thatREADME.mdis up to date.Description
Error Handling
powershell.instructions.mdfile. Now includes less error handling in the examples. This means when using the instructions file the output script contains less of the structured error handling, however the output scripts are easier for beginners and powershell novices to read and understand.Switch parameter
Examples updates
WhatIfparameter via$PSCmdlet.ShouldProcesss$Force&$PSCmdlet.ShouldContinuepatternType of Contribution
Additional Notes
I've tested the new instructions for a few months on and off. Many different models have been used, and I'm happier with the generated results of these instructions.
By submitting this pull request, I confirm that my contribution abides by the Code of Conduct and will be licensed under the MIT License.