Rule PSAvoidShouldContinueWithoutForce doesn't enforce correct use of the Force parameter #1308

Activity

1. JohnLBevan commented on Aug 10, 2019

   

   John Bevan (JohnLBevan)

   on Aug 10, 2019

   Author

   More actions

   Question: Does anyone have a preference on which line is flagged as in violation?

   • [CmdletBinding(SupportsShouldProcess = $true)]
   • [switch]$Force
   • $PSCmdlet.ShouldContinue('Should I continue','Should I continue'))

   i.e. as it's the combination of these three, plus the lack of the use of the force parameter further on which cause the issue.

   My instinct is to attach the violation to the [CmdletBinding(SupportsShouldProcess = $true)] line, as this is the line the violation for not having the Force parameter is attached to, so remains consistent; but I can see how it could be argued other ways.
2. JohnLBevan commented on Aug 10, 2019

   

   John Bevan (JohnLBevan)

   on Aug 10, 2019

   Last edited by JohnLBevan

   Author

   More actions

   Note: All of the following scenarios should be deemed valid:

   function Invoke-MVP {
       [CmdletBinding(SupportsShouldProcess = $true)] 
       [OutputType('System.String')]
       Param (
           [Parameter()]
           [switch]$Force
       )
       if ($Force -or $PSCmdlet.ShouldContinue('Should I continue','Should I continue')) {
           'I continued'
       }
   }

   function Invoke-MVP {
       [CmdletBinding(SupportsShouldProcess = $true)] 
       [OutputType('System.String')]
       Param (
           [Parameter(Mandatory = $true)]
           [string[]]$ComputerName
           ,
           [Parameter()]
           [switch]$Force 
       )
       [bool]$yesToAll = $Force
       [bool]$noToAll = $false
       foreach ($computer in $ComputerName) {
           if ($PSCmdlet.ShouldContinue('Should I continue','Should I continue', [ref]$yesToAll, [ref]$noToAll)) {
               "I continued: $computer"
           }
       }
   }

   In theory the below is also valid... but I suspect this is considered bad practice so should be reported as a violation / up to the coder to hide this via a SupressMessageAttribute attribute if they can justify going this route.

   function Invoke-MVP {
       [CmdletBinding(SupportsShouldProcess = $true)] 
       [OutputType('System.String')]
       Param (
           [Parameter()]
           [switch]$Force
       )
       if ($Force) {
               'I continued... She made me do it!'
       } else {
           if ($PSCmdlet.ShouldContinue('Should I continue','Should I continue')) {
               'I continued'
           }
       }
   }

   Note: I wouldn't have it treat $noToAll = $Force as acceptable, since that's counter intuitive. If people wanted an option for defaulting that they'd be better of using a, $AvoidClobber / $NoForce option; which is outside the scope of this rule.
3. bergmeister commented on Aug 15, 2019

   

   Christoph Bergmeister (bergmeister)

   on Aug 15, 2019

   Last edited by bergmeister

   Collaborator

   More actions

   I'd flag on [CmdletBinding(SupportsShouldProcess)]. You're right that the correct usage can be hard to determine, I originally thought that we could simply check for the presence of the -Force switch but remembered only now that we already have the AvoidShouldContinueWithoutForce rule for that. Unless you've got some use cases where we know for sure that the usage is not correct, then we might need to close the issue if it is too hard to accurately make a decision of the usage...

   👍React with 👍1Reacted by John Bevan
4. Christoph Bergmeister (bergmeister)

   added

   Up-for-Grabs

   on Aug 15, 2019
5. thomasrayner commented on Aug 30, 2019

   

   Thomas Rayner (thomasrayner)

   on Aug 30, 2019

   Last edited by thomasrayner

   Contributor

   More actions

   If you declare a parameter and then never use it, should you get a UseDeclaredVarsMoreThanAssignments violation (or something similar)? 🤔

   I know this is an issue about specifically using a $Force param if you have SupportsShouldProcess, but it made me wonder about the above.

   👍React with 👍1Reacted by Matt McNabb
6. mattmcnabb commented on Dec 8, 2019

   

   Matt McNabb (mattmcnabb)

   on Dec 8, 2019

   Contributor

   More actions

   If you declare a parameter and then never use it, should you get a UseDeclaredVarsMoreThanAssignments violation (or something similar)? 🤔

   That rule doesn't currently look at param blocks, but I think this is a good idea for a new rule. I have a POC of it that works so far but doesn't cover cases like $PSBoundParameters. Does anyone else agree that this might be a good rule to implement? If so I'll take a stab at it.