Skip to content

Add support for ValidateRangeKind to ParameterMetadata.GetProxyAttributeData#9059

Merged
anmenaga merged 8 commits intoPowerShell:masterfrom
indented-automation:master
May 7, 2019
Merged

Add support for ValidateRangeKind to ParameterMetadata.GetProxyAttributeData#9059
anmenaga merged 8 commits intoPowerShell:masterfrom
indented-automation:master

Conversation

@indented-automation
Copy link
Copy Markdown
Contributor

@indented-automation indented-automation commented Mar 5, 2019 •
edited by anmenaga
Loading

PR Summary

Fix #9058

  • ParameterMetadata: Add support for RangeKind to ParameterMetadata.GetProxyAttributeData method.
  • ProxyCommand: Add tests for GetParamBlock.

PR Context

The GetParamBlock method of ProxyCommand throws an unhandled exception when RangeKind is used.

The attributes came from #4084:

  • ValidatePositiveAttribute
  • ValidateNonNegativeAttribute
  • ValidateNegativeAttribute
  • ValidateNonPositiveAttribute

PR Checklist

@indented-automation indented-automation changed the title Adds support for ValidateRangeKind to ParameterMetadata.GetProxyAttributeData Add support for ValidateRangeKind to ParameterMetadata.GetProxyAttributeData Mar 5, 2019
@indented-automation indented-automation mentioned this pull request Mar 5, 2019
Closed
vexx32
vexx32 reviewed Mar 5, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Nicely done! I think it can be just a tad more robust, though!

src/System.Management.Automation/engine/Attributes.cs Outdated
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 Mar 5, 2019

Choose a reason for hiding this comment

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

Suggested change
RangeKind = kind;

And with the above, this line wouldn't be needed.

Copy link
Copy Markdown
Contributor Author

@indented-automation indented-automation Mar 5, 2019

Choose a reason for hiding this comment

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

Implemented.

@iSazonov iSazonov requested a review from lzybkr March 5, 2019 16:52
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Mar 5, 2019

@indented-automation Please add tests.

@indented-automation
Copy link
Copy Markdown
Contributor Author

indented-automation commented Mar 5, 2019

Sure. A new file in test\powershell\engine\Basic for ProxyCommand?

@indented-automation
Copy link
Copy Markdown
Contributor Author

indented-automation commented Mar 5, 2019

This method is going to fall foul of the new constructor for ValidateSet. Would you like to handle that as part of this PR? Or in a separate PR?

That change will be marginally more complex as the type used is discarded after the ValidateSet attribute is created at the moment.

iSazonov
iSazonov reviewed Mar 6, 2019
View reviewed changes
iSazonov
iSazonov reviewed Mar 6, 2019
View reviewed changes
@iSazonov iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Mar 6, 2019
iSazonov
iSazonov approved these changes Mar 6, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

Perhaps new test file must be in Basic folder.

@indented-automation
Copy link
Copy Markdown
Contributor Author

indented-automation commented Mar 13, 2019

Am I right in thinking I do not need to do anything about the quality failures? The suggestion is makes is not appropriate.

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Mar 13, 2019

If you ask about Codacy it reports false positives. Please ignore.

@anmenaga
Copy link
Copy Markdown

anmenaga commented Mar 25, 2019

@vexx32 can you please to another review for this? Thank you.

@stale
Copy link
Copy Markdown

stale bot commented Apr 24, 2019

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Apr 24, 2019
vexx32
vexx32 approved these changes Apr 24, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One minor comment about the surrounding code but I'm not sure it's entirely in scope.

Other than that, looks good to me @anmenaga!

(And sorry about the elapsed time, I'd lost track of this one somewhere.)

@stale stale bot removed the Stale label Apr 24, 2019
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Apr 25, 2019

@indented-automation Please rebase to get latest CI updates.

@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Apr 26, 2019 •
edited
Loading

@indented-automation looks like the merge pulled in some commits in a weird way. You might need to prune those commits out of the branch and do a proper rebase.

Assuming you have a remote called 'upstream' you can do it all in one go with git rebase -i upstream/master - drop all the commits you didn't do and lit it finish the rebase afterwards. You'll need to force-push once it's done.

@indented-automation
Copy link
Copy Markdown
Contributor Author

indented-automation commented Apr 26, 2019

Well that messed it right up. Bear with me here, knew that rebase was a bit too easy.

@iSazonov iSazonov added this to the 7.0.0-preview.1 milestone Apr 26, 2019
@anmenaga anmenaga merged commit 374e0cf into PowerShell:master May 7, 2019
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented May 8, 2019

@indented-automation Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@lzybkr lzybkr Awaiting requested review from lzybkr

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@anmenaga anmenaga Awaiting requested review from anmenaga

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@joeyaiello joeyaiello Awaiting requested review from joeyaiello

@mirichmo mirichmo Awaiting requested review from mirichmo

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

+1 more reviewer

@vexx32 vexx32 vexx32 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@anmenaga anmenaga

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

7.0.0-preview.1

Development

Successfully merging this pull request may close these issues.

GetParamBlock method of ProxyCommand fails

4 participants

@indented-automation @iSazonov @anmenaga @vexx32