-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Description
Prerequisites
- Existing Issue: Search the existing issues for this repository. If there is an issue that fits your needs do not file a new one. Subscribe, react, or comment on that issue instead.
- Descriptive Title: Write the title for this issue as a short synopsis. If possible, provide context. For example, "Typo in
Get-Foo
cmdlet" instead of "Typo." - Verify Version: If there is a mismatch between documentation and the behavior on your system, ensure that the version you are using is the same as the documentation. Check this box if they match or the issue you are reporting is not version specific.
Links
Summary
I was trying to understand the blog article's description and the code sample surrounding the use of $PSCmdlet.ShouldProcess
and the $Force
parameter. I originally read this article on Kevin's blog, and its quite a good article! Unfortunately, one of the code snippets showing an anti-pattern is very similar to the "correct way" to implementing -Force
with the use of $PSCmdlet.ShouldProcess
such that it has led to confusion for many readers, even myself on two separate occasions now!
Details
Under the section provided by the link to the documentation in this article, we have the following sample:
So we should implement
-Force
for the sanity of our users. Take a look at this full example here:function Test-ShouldProcess { [CmdletBinding( SupportsShouldProcess, ConfirmImpact = 'High' )] param( [Switch]$Force ) if ($Force){ $ConfirmPreference = 'None' } if ($PSCmdlet.ShouldProcess('TARGET')){ Write-Output "Some Action" } }
Later in that section, the code sample above is broken down into snippets and explained—well, at least the part about setting $ConfirPereference
. The next if
statement, as written above, is never discussed. Instead, the article goes on to explain an anti-pattern that looks extremely similar to the full sample above when implementing -Force
with $PSCmdlet.ShouldProcess
that leads many readers (including myself) to believe the full code sample has an error in it. It is not clear that this next snippet of code is, in fact, an anti-pattern other than a reference to an anti-pattern in the paragraph that follows the code snippet example, especially since prior to the anti-pattern example, the article was just finishing up it's explanation of setting $ConfirmPreference
to None
:
Focusing in on the -Force logic here:
if ($Force){ $ConfirmPreference = 'None' }If the user specifies
-Force
, we want to suppress the confirm prompt unless they also specify-Confirm
. This allows a user to force a change but still confirm the change. Then we set$ConfirmPreference
in the local scope. Now, using the-Force
parameter temporarily sets the$ConfirmPreference
to none, disabling prompt for confirmation.if ($Force -or $PSCmdlet.ShouldProcess('TARGET')){ Write-Output "Some Action" }If someone specifies both
-Force
and-WhatIf
, then-WhatIf
needs to take priority. This approach preserves-WhatIf
processing becauseShouldProcess
always gets executed.Do not add a check for the
$Force
value inside theif
statement with theShouldProcess
. That is an anti-pattern for this specific scenario even though that's what I show you in the next section forShouldContinue
.
I believe this happens more due to the flow of the article. The author seems to be breaking down the full code sample, but abruptly changes to discussing an anti-pattern with no indication that the next code sample is an anti-pattern and is distinctly different from the full code sample shown above.
Suggested Fix
Please fix the code snippet for implementing -Force
with the use of SupportsShouldProcess
showing an anti-pattern using the following diff as a reference:
Focusing in on the -Force logic here:
\```powershell
if ($Force){
$ConfirmPreference = 'None'
}
\```
If the user specifies `-Force`, we want to suppress the confirm prompt unless they also specify `-Confirm`.
This allows a user to force a change but still confirm the change. Then we set `$ConfirmPreference` in the local scope.
Now, using the `-Force` parameter temporarily sets the `$ConfirmPreference` to none, disabling prompt for confirmation.
+Next, we check the result of `ShouldProcess`:
+
+```powershell
+if ($PSCmdlet.ShouldProcess('TARGET')) {
+ Write-Output "Some Action"
+}
+```
-If someone specifies both `-Force` and `-WhatIf`, then `-WhatIf` needs to take priority. This
+Now if someone specifies both `-Force` and `-WhatIf`, then `-WhatIf` will have priority. This
approach preserves `-WhatIf` processing because `ShouldProcess` always gets executed.
+ It is an anti-pattern to add a check for the `$Force` value inside the `if` statement with the `ShouldProcess`,
+as shown here:
\```powershell
+# THIS IS AN ANTIPATTERN
+# Don't use $Force with ShouldProcess or you short-circuit WhatIf functionality!
if ($Force -or $PSCmdlet.ShouldProcess('TARGET')){
Write-Output "Some Action"
}
\```
## ShouldContinue -Force
With the anti-pattern clearly pointed out, the rest of the article becomes much clearer as to how -Force
is implemented with both ShouldProcess
and ShouldContinue
, including all the talk about "short-circuiting".