Skip to content
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

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

Closed
wants to merge 4 commits into from

Conversation

fflaten
Copy link
Collaborator

@fflaten fflaten commented Jul 30, 2022

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

  • PR has meaningful title
  • Summary describes changes
  • 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.
  • Tests are added/update (if required)
  • Documentation is updated/added (if required)

@fflaten fflaten marked this pull request as ready for review July 30, 2022 20:32
@nohwnd
Copy link
Member

nohwnd commented Aug 3, 2022

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?)

@fflaten
Copy link
Collaborator Author

fflaten commented Aug 3, 2022

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
Copy link
Collaborator Author

fflaten commented Aug 3, 2022

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

@fflaten fflaten marked this pull request as draft August 6, 2022 13:14
@fflaten
Copy link
Collaborator Author

fflaten commented Jan 20, 2023

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?

@nohwnd
Copy link
Member

nohwnd commented Apr 12, 2024

Sorry I've dropped the ball on this, do you want to finish it or should I close it?

@fflaten
Copy link
Collaborator Author

fflaten commented Apr 12, 2024

Depends on how you feel with the syntax. I'm fine with closing this, but the use case is valid and makes sense to support. Are we gonna have a Assert replacement for Should -HaveParameter? Using parameter sets, bool parameter or dynamic parameters would help with the syntax (avoiding -NonMandatory -Mandatory).

@nohwnd
Copy link
Member

nohwnd commented Apr 16, 2024

Yes there will be an alternative and I will be happy for your input on it :)

@fflaten fflaten closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Should -HaveParameter 'param' -Not -Mandatory" passes tests if parameter is not present
2 participants