-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: improve error handling in PowerShell guidelines #280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Update error handling examples to use $PSCmdlet.WriteError() and $PSCmdlet.ThrowTerminatingError() instead of Write-Error and throw for better PowerShell cmdlet integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves error handling practices in PowerShell cmdlet development guidelines by transitioning from basic PowerShell error handling methods to advanced function-specific approaches using $PSCmdlet
methods.
- Updated error handling guidelines to recommend
$PSCmdlet.WriteError()
and$PSCmdlet.ThrowTerminatingError()
overWrite-Error
andthrow
- Enhanced examples with proper ErrorRecord construction including error IDs, categories, and target objects
- Standardized string quotes from double to single quotes throughout examples
begin { | ||
Write-Verbose "Starting resource status update process" | ||
Write-Verbose 'Starting resource status update process' |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent quote style change. The original double quotes were appropriate and follow PowerShell conventions for strings that may contain variables or escape sequences. Consider keeping double quotes for consistency with existing PowerShell practices.
Write-Verbose 'Starting resource status update process' | |
Write-Verbose "Starting resource status update process" |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, expandable strings are only necessary when they contain variables or subexpressions. Otherwise, string literals are perfectly fine, and PSSA even has a rule (off by default) to convert any string literals to single quotes: PSAvoidUsingDoubleQuotesForConstantString
# Only output if PassThru is specified | ||
if ($PassThru) { | ||
if ($PassThru.IsPresent) { |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from if ($PassThru)
to if ($PassThru.IsPresent)
is unnecessary. PowerShell switch parameters evaluate to $true
when present and $false
when absent, making the .IsPresent
property redundant in boolean contexts.
if ($PassThru.IsPresent) { | |
if ($PassThru) { |
Copilot uses AI. Check for mistakes.
$errorRecord = [System.Management.Automation.ErrorRecord]::new( | ||
[System.Exception]::new("User account '$Username' not found"), | ||
'UserNotFound', | ||
[System.Management.Automation.ErrorCategory]::ObjectNotFound, | ||
$Username | ||
) | ||
$PSCmdlet.WriteError($errorRecord) |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Good improvement using $PSCmdlet.WriteError()
with proper ErrorRecord construction. However, consider using a more specific exception type like [System.ArgumentException]
instead of the generic [System.Exception]
for better error categorization.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering it categorizes it as ObjectNotFound, I'd say it's perfectly fine, unless you have a better .NET exception it could use? Perhaps [Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException]
?
Update error handling examples to use $PSCmdlet.WriteError() and $PSCmdlet.ThrowTerminatingError() instead of Write-Error and throw for better PowerShell cmdlet integration.
Pull Request Checklist
node update-readme.js
and verified thatREADME.md
is up to date.Description
Updates the error handling section and examples in the PowerShell cmdlet development guidelines to follow best practices for advanced functions.
Changes:
$PSCmdlet
methods overWrite-Error
andthrow
in advanced functionsRemove-UserAccount
example to demonstrate$PSCmdlet.WriteError()
for non-terminating errors and$PSCmdlet.ThrowTerminatingError()
for terminating errorsNew-Resource
example to use$PSCmdlet.ThrowTerminatingError()
with proper ErrorRecord constructionThese changes ensure GitHub Copilot generates PowerShell code with better error reporting and integration with PowerShell's error handling pipeline.
Type of Contribution
Additional Notes
This update improves the existing PowerShell guidelines (example.md) to align more closely with Microsoft's recommended patterns for advanced function error handling. The use of
$PSCmdlet.WriteError()
and$PSCmdlet.ThrowTerminatingError()
provides better integration with PowerShell's error handling pipeline and allows for more detailed error categorization.By submitting this pull request, I confirm that my contribution abides by the Code of Conduct and will be licensed under the MIT License.