8000 Remove [ValidateNotNullOrEmpty] from -InputObject on Get-Random to allow empty string by SteveL-MSFT · Pull Request #10644 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Remove [ValidateNotNullOrEmpty] from -InputObject on Get-Random to allow empty string #10644

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.

Sign up for GitHub

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

Jump to bottom
Merged
4 commits merged into from
Oct 1, 2019
Merged

Remove [ValidateNotNullOrEmpty] from -InputObject on Get-Random to allow empty string #10644

4 commits merged into from
Oct 1, 2019

Conversation

@SteveL-MSFT
Copy link
Member
@SteveL-MSFT SteveL-MSFT commented Sep 28, 2019 •
edited
Loading

PR Summary

Get-Random has [ValidateNotNullOrEmpty] attribute on InputObject parameter which limits a collection from having an empty string. However, if you use the default positional parameter Maximum it doesn't have this restriction. This creates confusion to users as they expect these two to be equivalent:

Get-Random @('a','') # this already worked
Get-Random -InputObject @('a','')

Although it's not obvious that this is two different parameter sets. However, there's no reason to restrict InputObject parameter here. Since it's mandatory, you can't give it $null only.

This change also allows $null in the collection:

Get-Random @('a',$null) # this already worked
Get-Random -InputObject @(`a`,$null)

PR Context

Fix #3682

PR Checklist

iSazonov
iSazonov reviewed Sep 30, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs
foreach (object item in InputObject ?? _nullInArray)
{
if (_numberOfProcessedListItems < Count) // (3)
// (3)
Copy link
Collaborator
@iSazonov iSazonov Sep 30, 2019

Choose a reason for hiding this comment

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

What is the comment for?

Copy link
Member Author
@SteveL-MSFT SteveL-MSFT Sep 30, 2019

Choose a reason for hiding this comment

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

Codefactor was complaining about the trailing comment so I moved it above. If you look at the large comment above, it references (1), (2), (3) explaining the code so I left it.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 30, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.5 milestone Sep 30, 2019
@mklement0
Copy link
Contributor
mklement0 commented Sep 30, 2019

Thanks for the quick turnaround, @SteveL-MSFT and @iSazonov.

@SteveL-MSFT, to avoid confusion, could you please update the OP to indicate that $null values in the input collection now are accepted (in addition to empty strings)?

@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Oct 1, 2019
@iSazonov
Copy link
Collaborator
iSazonov commented Oct 1, 2019

It seems we need an issue in Doc repo too.

@SteveL-MSFT SteveL-MSFT mentioned this pull request Oct 1, 2019
Closed
8 tasks
@SteveL-MSFT
Copy link
Member Author
SteveL-MSFT commented Oct 1, 2019

@iSazonov added

@iSazonov iSazonov added AutoMerge informs the bot to automerge the PR and removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 1, 2019
@ghost
Copy link
ghost commented Oct 1, 2019

Hello @iSazonov!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit d8eca6a into PowerShell:master Oct 1, 2019
@iSazonov
Copy link
Collaborator
iSazonov commented Oct 1, 2019 •
edited
Loading

Oops, I thought AutoMerge is waiting for the day before merge 😕

@ghost
Copy link
ghost commented Oct 23, 2019

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Oct 23, 2019
Closed
@SteveL-MSFT SteveL-MSFT deleted the get-random branch June 6, 2020 02:31
This pull request was closed.
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

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

+2 more reviewers

@mklement0 mklement0 mklement0 left review comments

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

AutoMerge informs the bot to automerge the PR CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.0.0-preview.5

Development

Successfully merging this pull request may close these issues.

Strange Get-Random Input-Object parameter behaviour

4 participants

@SteveL-MSFT @mklement0 @iSazonov @PoshChan
0