Implement Get-Random -Count without specifying an InputObject list #9111

pougetat · 2019-03-11T13:25:03Z

PR Summary

The purpose of this PR is to implement the following feature for the Get-Random commandlet :

Get-Random -Maximum 10 -Count 3
5
1
7

PR Context

This PR is tied to the following issue :
fix #8986

PR Checklist

vexx32

Looks good, been wanting to get this done, so thanks! 😄

vexx32 · 2019-03-11T13:31:54Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs

-                    int randomNumber = this.Generator.Next(min, max);
-                    Debug.Assert(min <= randomNumber, "lower bound <= random number");
-                    Debug.Assert(randomNumber < max, "random number < upper bound");
+                    for (int i = 0; i < Math.Max(1, this.Count); i++)


Rather than need a call to Math.Max() you could simply initialize the value of this parameter to 1 by default.

vexx32 · 2019-03-11T13:32:07Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs

@@ -281,7 +281,7 @@ private double ConvertToDouble(object o, double defaultIfNull)
        /// <summary>
        /// Number of items to output (number of list items or of numbers).
        /// </summary>
-        [Parameter(ParameterSetName = GetRandomCommand.RandomListItemParameterSet)]
+        [Parameter]
        [ValidateRange(1, int.MaxValue)]
        public int Count { get; set; }


Suggested change

public int Count { get; set; }

public int Count { get; set; } = 1;

vexx32 · 2019-03-11T13:34:45Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs

+                    for (int i = 0; i < Math.Max(1, this.Count); i++)
+                    {
+                        int randomNumber = this.Generator.Next(min, max);
+                        Debug.Assert(min <= randomNumber, "lower bound <= random number");


I don't think we generally use Debug.Assert() in cmdlets. This seems like something that should only fail if the Random class has a fatal regression for its Next() code path(s), which is outside the purview of our debug code in general, I would think.

I think if we want to test this at all, we should implement this as a Pester test so that we can see it in CI rather than waiting for someone to stumble across it. 🙂

Hmm not too sure I agree that Debug.Assert isn't used since I have found it in quite a few cmdlets throughout the codebase.
I do agree that ideally this type of thing should surface as a result of a failing CI test but I don't know how important the consequence of removing it would be (scripts not crashing and using potentially invalid random values). 🙂

@JamesWTruher ? @PaulHigin ?

If I remember rightly @PaulHigin, aren't Debug.Assert() calls ignored / no-op in release builds anyway? 🤔 Or am I misremembering? 😄

I agree that this is the wrong place for debug asserts and the correct place is in the random number generator adapter. Debug asserts are only evaluated in debug builds and are intended to verify expected (and assumed) behavior.

vexx32 · 2019-03-11T13:39:36Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs

-                    Debug.Assert(randomNumber < max, "random number < upper bound");
+                    for (int i = 0; i < Math.Max(1, this.Count); i++)
+                    {
+                        int randomNumber = this.Generator.Next(min, max);


I don't think this. is usually needed here. Is there a naming conflict that requires this? If not, we don't generally use it where it's not explicitly required. 🙂

There are a couple of these, just tagging this one.

Done. I've removed all this. invocations from this commandlet. :)

Cheers! 😄

PaulHigin

LGTM

PaulHigin · 2019-03-11T16:10:50Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs

+                    for (int i = 0; i < Math.Max(1, this.Count); i++)
+                    {
+                        int randomNumber = this.Generator.Next(min, max);
+                        Debug.Assert(min <= randomNumber, "lower bound <= random number");


I agree that this is the wrong place for debug asserts and the correct place is in the random number generator adapter. Debug asserts are only evaluated in debug builds and are intended to verify expected (and assumed) behavior.

iSazonov · 2019-03-14T04:48:34Z

@pougetat Please move all style changes in new PR - we should not mix style and function changes.

Also please open new issue in PowerShell-Docs repo and add reference to the PR description (current link points to PowerShell repo issue that is wrong).

pougetat · 2019-03-14T09:08:07Z

@iSazonov
I have opened #9133 to fix all the coding style issues. As soon as that one is merged I will checkout this one and rebase master.

pougetat · 2019-03-16T10:05:08Z

Hey @iSazonov ,
I rebased master containing the PR you just merged with the codestyle changes. This PR now only contains a single clean commit with the Feature implementation. Also, I haven't touched the asserts. :)

iSazonov · 2019-03-17T08:08:44Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1

+
+        $result = Get-Random -Maximum $maximum -Minimum $minimum -Count 1
+        $result | Should -BeGreaterThan $greaterThan
+        $result | Should -BeLessThan $lessThan


Looking asserts above it seems it should be BeLessOrEqual in all tests. What do you think?

I agree with you. I've made the modifications.

iSazonov · 2019-03-18T04:16:27Z

@PaulHigin @adityapatwardhan Please confirm latest changes in tests.

adityapatwardhan · 2019-03-18T19:10:11Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1

-        @{ Name = 'max is double with plus sign and enclosed in quote'; Maximum = '+100.0'; Minimum = 0; GreaterThan = -1.0; LessThan = 100.0; Type = 'System.Double' }
-        @{ Name = 'both set to the special numbers as 1.0e+xx '; Maximum = $null; Minimum = 1.0e+100; GreaterThan = 1.0e+99; LessThan = ([double]::MaxValue); Type = 'System.Double' }
-        @{ Name = 'max is Double.MaxValue, min is Double.MinValue'; Maximum = ([double]::MaxValue); Minimum = ([double]::MinValue); GreaterThan = ([double]::MinValue); LessThan = ([double]::MaxValue); Type = 'System.Double' }
+        @{ Name = 'no params'; Maximum = $null; Minimum = $null; GreaterThan = -1; LessOrEqual = ([int32]::MaxValue); Type = 'System.Int32' }


Why was LessThan changed to LessOrEqual?

The debug.assert statements in the code a exlusive for lower bounds and inclusive for higher bounds thus we have changed LessThan into LessOrEqual to make it consistent with the current implementation.

Yes, our tests do not match the code. One of the two must be corrected.

Wait sorry i mixed things up. The assert statements are exclusive for higher bounds and inclusive for lower bounds. In any case I think I should just revert the changes made to the tests for the following reason :

The debug.assert statements should be moved into the actual random number generators but for that it also means looking at what numbers can be generated (inclusive bounds VS exclusive bounds) since some of the bounding conditions will be incorrect as they stand today.
I think that discussion can be had in another PR and this one can simply remain very pointed in scope :)
I have no issue opening the extra PR myself btw.

@pougetat Let's wait @adityapatwardhan conclusion.

@pougetat Sorry I reverted the commit when I wrote my previous comment. Having said that I still have it on a local branch so I can put it back easily depending on his conclusion. I should have simply reverted with an actual commit instead of dropping the commit altogether. In any case yes I agree we can wait for his conclusion :).

I agree with having a separate PR for that. I just signed off.

@pougetat for future reference, I do believe the random generators are inclusive on the lower bound, but exclusive on the higher bound. Don't recall the exact reason for it, but it's a pretty common pattern.

It is documented https://docs.microsoft.com/en-us/dotnet/api/system.random.next?redirectedfrom=MSDN&view=netcore-2.2#System_Random_Next_System_Int32_System_Int32_

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1

iSazonov · 2019-03-21T12:40:08Z

@mklement0 Does this meet your expectations?

mklement0 · 2019-03-21T14:04:19Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1

@@ -163,6 +178,7 @@ Describe "Get-Random" -Tags "CI" {
        $secondRandomNumber = Get-Random 34359738367 -SetSeed 20
        $firstRandomNumber | Should -Be @secondRandomNumber
    }
+
    It "Should throw an error because the hexadecimal number is to large " {
        { Get-Random 0x07FFFFFFFFFFFFFFFF } | Should -Throw "Value was either too large or too small for a UInt32"


I haven't dug deeper, but the full error message is self-contradictory (Int32 vs. UInt32): Cannot convert value "0x07FFFFFFFFFFFFFFFF" to type "System.Int32". Error: "Value was either too large or too small for a UInt32." Where does the UInt come from?

Oh, I actually fixed this back when I was toying with the tokenizer, and then it was requested I re-break this for consistency with prior implementations. This can be fixed, but the fix would be in the tokenizer and how hex numbers get parsed.

This number is an extra digit beyond the widest permitted for uint64, and a hex number with a leading zero is forced to be an unsigned numeral when using the conversion paths we have in the tokenizer (which are still done with Convert.ToInt32() I think?). I think the default path goes to (u)int32 because it checks for (u)int64 lengths and then defaults to (u)int32 for all other cases iirc.

I remember I originally did some fun things in my tokenizer PRs where I would automatically upcast between uint/int64 if needed, but it differed too much from the previous implementation I think and we decided to keep the behaviour consistent. I think my solution was to have it upcast up to decimal (or even biginteger) if needed, which worked well.

I think due to the refactoring in #7993 this error message might actually change, but it's been a little while and I don't quite recall.

I see: if the pre-parameter-binding typing can't convert a "number-looking" argument to an actual number, it passes a string, and line baseObject = System.Management.Automation.Language.Parser.ScanNumber((string)baseObject, typeof(int)); then re-attempts conversion, which fails.

Again, not in the scope of this PR, but I wonder if the error message could be made friendlier (and it's still ~~curious that UInt32 is referenced, given that hex. literals seem to top out at Int64~~ [see previous comment]).

Oops, @vexx32 - wires crossed - reading your comment now.

mklement0

The following is unrelated to this specific PR, but it's worth pointing out:

Tests such as the following are flawed, because they always pass:

$randomNumber[0] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)

The above should be:

$randomNumber[0] | Should -BeIn 1, 2, 3, 5, 8, 13

mklement0 · 2019-03-21T15:10:18Z

Thanks for asking, @iSazonov: looks great. Thanks for taking this on, @pougetat.

iSazonov · 2019-03-26T11:50:50Z

@pougetat Thanks for your contribution!

pougetat · 2019-03-26T12:19:16Z

@iSazonov Thanks for the review

pougetat requested review from JamesWTruher and PaulHigin as code owners March 11, 2019 13:25

vexx32 reviewed Mar 11, 2019

View reviewed changes

PaulHigin approved these changes Mar 11, 2019

View reviewed changes

iSazonov self-assigned this Mar 14, 2019

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 14, 2019

pougetat mentioned this pull request Mar 14, 2019

Clean up Get-Random cmdlet #9133

Merged

11 tasks

pougetat mentioned this pull request Mar 14, 2019

Document new -Count without InputObjects list for Get-Random cmdlet MicrosoftDocs/PowerShell-Docs#3951

Closed

8 tasks

Improve Get-Random by allowing -Count without input objects list

3fd48e6

pougetat force-pushed the improve-getrandom branch from ec13067 to 3fd48e6 Compare March 16, 2019 09:55

iSazonov reviewed Mar 17, 2019

View reviewed changes

iSazonov requested a review from adityapatwardhan March 18, 2019 04:15

adityapatwardhan requested changes Mar 18, 2019

View reviewed changes

pougetat force-pushed the improve-getrandom branch from ae1372d to 3fd48e6 Compare March 19, 2019 08:31

adityapatwardhan approved these changes Mar 20, 2019

View reviewed changes

iSazonov reviewed Mar 21, 2019

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1 Show resolved Hide resolved

mklement0 reviewed Mar 21, 2019

View reviewed changes

TravisEz13 added this to the 6.3.0-preview.1 milestone Mar 22, 2019

Addressing PR comments

2b98988

iSazonov merged commit e553aef into PowerShell:master Mar 26, 2019

	public int Count { get; set; }
	public int Count { get; set; } = 1;

Implement Get-Random -Count without specifying an InputObject list #9111

Implement Get-Random -Count without specifying an InputObject list #9111

Conversation

pougetat commented Mar 11, 2019 • edited by iSazonov

PR Summary

PR Context

PR Checklist

vexx32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulHigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Mar 14, 2019 • edited

pougetat commented Mar 14, 2019 • edited by iSazonov

pougetat commented Mar 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pougetat Mar 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mklement0 Mar 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mklement0 left a comment • edited

Choose a reason for hiding this comment

mklement0 commented Mar 21, 2019

iSazonov commented Mar 26, 2019

pougetat commented Mar 26, 2019

pougetat commented Mar 11, 2019 •

edited by iSazonov

iSazonov commented Mar 14, 2019 •

edited

pougetat commented Mar 14, 2019 •

edited by iSazonov

pougetat Mar 19, 2019 •

edited

mklement0 Mar 21, 2019 •

edited

mklement0 left a comment •

edited