Add -Shuffle switch to Get-Random command
#11093
Conversation
vexx32
left a comment
vexx32
left a comment
There was a problem hiding this comment.
Couple minor comments, looks pretty solid overall.
Nice work! 😊 💖
| $randomNumber[0] | Should -BeIn 1, 2, 3, 5, 8, 13 | ||
| $randomNumber[1] | Should -BeIn 1, 2, 3, 5, 8, 13 | ||
| $randomNumber[2] | Should -BeIn 1, 2, 3, 5, 8, 13 | ||
| $randomNumber[3] | Should -BeIn 1, 2, 3, 5, 8, 13 | ||
| $randomNumber[4] | Should -BeIn 1, 2, 3, 5, 8, 13 | ||
| $randomNumber[5] | Should -BeIn 1, 2, 3, 5, 8, 13 |
There was a problem hiding this comment.
From memory you can shorten these tests to just $randomNumber | Should -BeIn 1, 2, 3, 5, 8, 13
| #region Shuffle parameter | ||
|
|
||
| /// <summary> | ||
| /// If specified, then the command will return all input objects in randomized order. |
There was a problem hiding this comment.
You'll need to phrase this with Gets or sets whether the command (...) in order to satisfy CodeFactor.
At some point we'll have to go back through and align the other parameter descriptions with that format, but no need to go that far for this PR. 🙂
|
|
||
| private const string RandomNumberParameterSet = "RandomNumberParameterSet"; | ||
| private const string RandomListItemParameterSet = "RandomListItemParameterSet"; | ||
| private const string ShuffleSet = "ShuffleSet"; |
There was a problem hiding this comment.
| private const string ShuffleSet = "ShuffleSet"; | |
| private const string ShuffleSet = "ShuffleParameterSet"; |
Just to keep it in the same vein as the existing parameter set names, really. 🙂
| else if (ParameterSetName.Equals(GetRandomCommand.RandomListItemParameterSet, StringComparison.OrdinalIgnoreCase) || | ||
| ParameterSetName.Equals(GetRandomCommand.ShuffleSet, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Where a condition like this is necessary, most other (recent) areas of the codebase have it with the || starting the following line. Also, you can just do a straight == on these. Not sure why the original code bothered to check without case sensitivity; the parameter set names shouldn't end up in a different casing to how they're defined as far as I'm aware. Something like the following should be sufficient:
else if (ParameterSetName == GetRandomCommand.RandomListItemParameterSet
|| ParameterSetName == GetRandomCommand.ShuffleSet)There was a problem hiding this comment.
Honestly just in general I have no idea why this code is written in this way, I can't think of a case where this is especially important but no reason to go changing the rest of this now I suppose. Never really thought checking parameter set names would be performance-intensive. 🤔
ghost
added
the
Waiting on Author
ghost
removed
the
Waiting on Author
|
Thanks for the review! Fixed. |
src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs
Show resolved
Hide resolved
iSazonov
added
the
CL-General
adityapatwardhan
changed the title
-Shuffle switch to Get-Random command-Shuffle switch to Get-Random command
|
@eugenesmlv Thank you for your contribution! |
|
🎉 Handy links: |
ghost
mentioned this pull request
4 participants
PR Summary
Added
-Shuffleswitch to Get-Random command. Also added test for this feature.PR Context
Fix #11022
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.