Pester icon indicating copy to clipboard operation
Pester copied to clipboard

Allow -Mandatory:$false and -HasArgumentCompleter:$false in HaveParameter assertion

Open fflaten opened this issue 3 years ago • 4 comments

PR Summary

Enables Should -HaveParameter to test that a parameter actually exists, but was not mandatory and/or did not have argument completer.

Currently this was only covered by Should -Not -HaveParameter abc -Mandatory -HasArgumentCompleter which would also pass if the parameter was missing, requiring a leading Should -HaveParameter abc assertion to avoid false positive.

The change does not break existing syntax and -SwitchParam:$false syntax is compatible a bool-parameter should it be changed in the future. So I don't believe this commits us to anything in the future that would require us to wait for new Should/Assert.

Fix #2105

PR Checklist

  • [x] PR has meaningful title
  • [x] Summary describes changes
  • [x] PR is ready to be merged
    • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
  • [x] Tests are added/update (if required)
  • [x] Documentation is updated/added (if required)

fflaten avatar Jul 30 '22 15:07 fflaten

This is great, but seems inconsistent with the rest of our parameters. I understand it is consistent with PowerShell, but not with Pester syntax.

I feel like there should be special Should -HaveParameter abc -NonMandatory -HasNotArgumentCompleter parameters. (better names?)

nohwnd avatar Aug 03 '22 05:08 nohwnd

I agree it's not ideal syntax, but what is? Do we have any -NotXyz parameters in an operator now? Even with new Should-HaveParameter function, we'd get parameter set conflicts. We'd need four parameter sets to make different combinations of the -*Mandatory and -*ArgumentCompleter switches work, but then it would throw on ex. -Mandatory due to insufficient parameters (missing -NotArgumentCompleter or -ArgumentCompleter) to calculate set.

We could use -NotMandatory etc with our own conflict-test to throw if used with -Mandtory and -Not. Might be a little confusing since intellisense/tabcomplete will suggest the conflicts, but at least the parameternames make the conflict obvious. Good enough?

This is just submitted as a suggestion and I'm also fine with closing it and revisit later.

fflaten avatar Aug 03 '22 08:08 fflaten

In case you read the comment above early, I've made some edits. 🙂

fflaten avatar Aug 03 '22 10:08 fflaten

We could use -NotMandatory etc with our own conflict-test to throw if used with -Mandtory and -Not. Might be a little confusing since intellisense/tabcomplete will suggest the conflicts, but at least the parameternames make the conflict obvious. Good enough?

@nohwnd Better? Keep current? Close and fix later?

fflaten avatar Jan 20 '23 10:01 fflaten