NotificationsYou must be signed in to change notification settings
Fork8.1k
Star51k

Respect $VerbosePreference in ShouldProcess#26511

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

yotsuda wants to merge3 commits intoPowerShell:master

base:master

Choose a base branch

fromyotsuda:fix/issue-12148-shouldprocess-verbosepreference

Open

Respect $VerbosePreference in ShouldProcess#26511

yotsuda wants to merge3 commits intoPowerShell:masterfromyotsuda:fix/issue-12148-shouldprocess-verbosepreference

+104 −4

Conversation

Copy link

Contributor

yotsuda commentedNov 22, 2025

PR Summary

FixShouldProcess to respect$VerbosePreference variable in addition to-Verbose parameter.

PR Context

Fixes#12148

$PSCmdlet.ShouldProcess() was only emitting verbose output when the-Verbose parameter was explicitly specified, completely ignoring the$VerbosePreference variable setting. This is inconsistent with how other cmdlets handle verbose output.

The issue occurred becauseDoShouldProcess() andCalculatePossibleShouldProcessOptimization() were checkingthis.Verbose (which only tracks the-Verbose parameter flag) instead of callingIsWriteVerboseEnabled() (which properly checks both-Verbose parameter and$VerbosePreference variable).

Changes:

ModifiedMshCommandRuntime.cs to useIsWriteVerboseEnabled() instead ofthis.Verbose in two locations
Added comprehensive tests to verify the fix

Before:

functionTest-SP {    [CmdletBinding(SupportsShouldProcess)]param($Target="TestTarget",$Action="TestAction")if ($PSCmdlet.ShouldProcess($Target,$Action)) {"OK" }}$VerbosePreference='Continue'Test-SP# No verbose output (bug)

After:

functionTest-SP {    [CmdletBinding(SupportsShouldProcess)]param($Target="TestTarget",$Action="TestAction")if ($PSCmdlet.ShouldProcess($Target,$Action)) {"OK" }}$VerbosePreference='Continue'Test-SP# VERBOSE: Performing the operation "TestAction" on target "TestTarget". (fixed!)

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all.h,.cpp,.cs,.ps1 and.psm1 files have the correct copyright header
This PR is ready to merge. If this PR is a work in progress, please open this as aDraft Pull Request and mark it as Ready to Review when it is ready to merge.
Breaking changes
- None
- OR
- Experimental feature(s) needed
  - Experimental feature name(s):
User-facing changes
- Not Applicable
- OR
- Documentation needed
  - Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed

FixPowerShell#12148: Make ShouldProcess respect $VerbosePreference

6898479

ShouldProcess was only emitting verbose output when the -Verboseparameter was explicitly specified, ignoring the $VerbosePreferencevariable setting.Changed to use IsWriteVerboseEnabled() instead of checking theVerbose flag directly in both DoShouldProcess() andCalculatePossibleShouldProcessOptimization(). This method properlyrespects both the -Verbose parameter and $VerbosePreference variable.

iSazonov added the CL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change Log label

Nov 22, 2025

Copy link

Collaborator

iSazonov commentedNov 22, 2025

From#12148 (comment)

ShouldProgress needs to respect $ProgressPreference = 'Continue', but if -Verbose:$false, then that should take precedent.

@yotsuda Could you please add test for-Verbose:$false?

iSazonov reviewed

Nov 22, 2025

View reviewed changes

test/powershell/Language/Scripting/ShouldProcess.Tests.ps1 Outdated


		Context"VerbosePreference variable" {
		It"Emits verbose output when VerbosePreference is Continue" {
		$VerbosePreference='Continue'

Copy link

Collaborator

iSazonovNov 22, 2025

Choose a reason for hiding this comment

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

Please save and then restore the value.

Copy link

ContributorAuthor

yotsudaNov 22, 2025

Choose a reason for hiding this comment

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

Thanks for the review! Fixed in5d8a7d1. All tests now save and restore $VerbosePreference using try-finally blocks.

Add test for -Verbose:$false and fix VerbosePreference state management

5d8a7d1

- Add test case for -Verbose:$false taking precedence over $VerbosePreference- Save and restore $VerbosePreference in all test cases using try-finally blocks- Clean up whitespace

iSazonov requested a review fromCopilot

November 22, 2025 16:54

Copilotstarted reviewing on behalf ofiSazonov

November 22, 2025 16:54

View session

Copilotfinished reviewing on behalf ofiSazonov

November 22, 2025 16:56

CopilotAI reviewed

Nov 22, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a 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 fixes a bug whereShouldProcess was only emitting verbose output when the-Verbose parameter was explicitly specified, completely ignoring the$VerbosePreference variable. This makes the behavior consistent with how other cmdlets handle verbose output by properly checking both the parameter and the preference variable.

Key Changes:

Changed two locations inDoShouldProcess() andCalculatePossibleShouldProcessOptimization() from checkingthis.Verbose to callingIsWriteVerboseEnabled()
Added comprehensive test coverage for both$VerbosePreference variable and-Verbose parameter scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
`src/System.Management.Automation/engine/MshCommandRuntime.cs`	Fixed two method calls to use`IsWriteVerboseEnabled()` instead of`this.Verbose` to properly respect both`-Verbose` parameter and`$VerbosePreference` variable
`test/powershell/Language/Scripting/ShouldProcess.Tests.ps1`	Added new test file with comprehensive coverage for`ShouldProcess` verbose output behavior with both preference variable and parameter scenarios

Comments suppressed due to low confidence (1)

src/System.Management.Automation/engine/MshCommandRuntime.cs:1649

Both branches of this 'if' statement return - consider using '?' to express intent better.

                if (IsWriteVerboseEnabled())                {                    return ShouldProcessPossibleOptimization.AutoYes_CanCallShouldProcessAsynchronously;                }                else                {                    return ShouldProcessPossibleOptimization.AutoYes_CanSkipShouldProcessCall;                }

💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.

Copy link

Collaborator

iSazonov commentedNov 24, 2025

@yotsuda Please look test errors.

yotsuda marked this pull request as draft

November 24, 2025 22:44

Fix ShouldProcess verbose to respect only VerbosePreference and -Verbose

ce12d77

- Add ShouldEmitVerboseFromShouldProcess() method that checks only  -Verbose parameter and $VerbosePreference variable- Excludes -Debug parameter effect to avoid unintended side effects- Update History.Tests.ps1 expected count from 12 to 13 to reflect  the correct behavior when $VerbosePreference is 'Continue'

yotsuda marked this pull request as ready for review

November 26, 2025 04:28

Copy link

ContributorAuthor

yotsuda commentedNov 26, 2025

@yotsuda Please look test errors.

Fixed ince12d77. The original implementation usingIsWriteVerboseEnabled() was affected by-Debug parameter, causing unintended behavior.

Created a dedicated methodShouldEmitVerboseFromShouldProcess() that checks only:

-Verbose parameter (if explicitly set)
$VerbosePreference variable

Also updatedHistory.Tests.ps1 expected count (12 → 13) as it now correctly emits verbose output.
All CI checks passing.

Copy link

Collaborator

iSazonov commentedNov 26, 2025

The original implementation usingIsWriteVerboseEnabled() was affected by-Debug parameter, causing unintended behavior.

Hmm,IsWriteVerboseEnabled is not directly check Debug parameter. So it is not right fix.

Copy link

ContributorAuthor

yotsuda commentedNov 27, 2025

Hmm,IsWriteVerboseEnabled is not directly check Debug parameter. So it is not right fix.

@iSazonov You're right thatIsWriteVerboseEnabled() doesn't directly check-Debug. However, it uses theVerbosePreference property getter, which returnsContinue/Inquire when-Debug is set (line 3116-3128), before checking$VerbosePreference.

TheWG decision specifies only-Verbose and$VerbosePreference - not-Debug. So I createdShouldEmitVerboseFromShouldProcess() to check these two factors directly, bypassing the-Debug logic in the property getter.

Copy link

Collaborator

iSazonov commentedNov 29, 2025

Thanks for clarify!

I don't see that WG discussed the influence of-Debug switch presence on the issue. I believe that this parameter is taken into account in all preferences for a reason. We should probably ask for confirmation from the WG in original issue.