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

HaveParameter is not dependent on format of param block #2288

Merged
merged 5 commits into from
Mar 31, 2023

Conversation

brianwest
Copy link
Contributor

@brianwest brianwest commented Jan 24, 2023

PR Summary

Get-ParameterInfo wasn't leveraging ASTs because of previous support for PowerShell v2 and required that the opening parenthesis for the param block be on the same line as the keyword. This change leverages ASTs for Get-ParameterInfo so that HaveParmeter is format agnostic.

Fix #2285

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)

@nohwnd
Copy link
Member

nohwnd commented Jan 25, 2023

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@brianwest brianwest marked this pull request as ready for review January 25, 2023 14:12
Copy link
Collaborator

@fflaten fflaten left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this issue!
This looks great - and so much simpler! I've added a few suggestions.

Would you mind also adding a test in tst/functions/assertions/HaveParameter.Tests.ps1?
Ex the one you provided in the issue comment?

src/functions/assertions/HaveParameter.ps1 Outdated Show resolved Hide resolved
src/functions/assertions/HaveParameter.ps1 Show resolved Hide resolved
src/functions/assertions/HaveParameter.ps1 Outdated Show resolved Hide resolved
@azure-pipelines
Copy link
Contributor

Commenter does not have sufficient privileges for PR 2288 in repo pester/Pester

@fflaten
Copy link
Collaborator

fflaten commented Feb 18, 2023

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@fflaten fflaten left a comment

Choose a reason for hiding this comment

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

Sorry for the late response. See updated comments 🙂

src/functions/assertions/HaveParameter.ps1 Outdated Show resolved Hide resolved
src/functions/assertions/HaveParameter.ps1 Outdated Show resolved Hide resolved
…ded dedicated test for paramblock with opening paren on new line
Copy link
Collaborator

@fflaten fflaten left a comment

Choose a reason for hiding this comment

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

LGTM!

@fflaten fflaten added the Bug label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HaveParameter DefaultValue evaluates to '<empty>' when actual value is not empty
3 participants