Moving New-Guid to compiled cmdlet by powercode · Pull Request #2752 · PowerShell/PowerShell

powercode · Nov 21, 2016

Fixing #2733
Simplest implementation possible

msftclas · Nov 21, 2016

Hi @powercode, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

lzybkr · Nov 21, 2016

Empty should be all zeros. Fixed.

lzybkr · Nov 21, 2016

We indent with spaces, not tabs.

How I wish that tab/spaces was a per project setting in VS...
DICE has tabs, and switching back and forth is a mess...

lzybkr · Nov 21, 2016

The cmdlet suggests that it creates a new object every time it's called, but with this parameter, nothing is actually created.

Well, you have noting before, and after you have a guid. Admittedly a static one that will never change, but it is friendlier to type new-guid -e than [guid]::empty, especially with Scandinavian keyboards, where '[' is on AltGr+8.

But it's your call. Remove?

lzybkr · Nov 21, 2016

This code feels like it doesn't belong in a cmdlet with the verb New, and I don't think we want a cmdlet Convert-ToGuid.

(Copy from Issue) Some applications use GUID as a unique identifier. In this case, the identifier is a string. So really, we can get a stream of strings (ex. from logs), that we want to convert to [guid] . And yes this is more for ConvertTo-Guid but I looked through New-* cmdlets and many of them do a similar operation (string -> object). Most likely the new ConvertTo-Guid cmdlet is redundant and perhaps it would be better to make the GUID parameter.

vors · Nov 21, 2016

You don't need to update map.json for new files.

iSazonov · Nov 22, 2016

It seems this parameter is redundant.
The only cmdlet that uses Count parameter is Get-Random and only to determines how many objects are returned from input pipeline.
Usually we do so:

0..9 | New-Guid

Well, to me, that looks like the input in some way should be related to the created guids.

Yes, the sample is incorrect.
I mean:

0..10 | % { Get-Random }

iSazonov · Nov 22, 2016

Most likely you had in mind:
if (String.Equals(ParameterSetName, "Empty", StringComparison.OrdinalIgnoreCase))
or
if (ParameterSetName == "Empty")

Well, the empty switch will only be on if the parameterset is "Empty" so it seems like a cheaper and simpler test. They are functionally equivalent, but I think this is easier to read

Using if (Empty) here will also handle a rare usage of the cmdlet: New-Guid -Empty:$false -- it will write out a new GUID in that case.

daxian-dbw · Nov 23, 2016

This comment probably should be Return one or more guids.

lzybkr · Nov 24, 2016

Don't merge yet - I've marked this for committee review.

I personally wouldn't mind removing this command, or if we do, it should work just like the existing function, nothing more, but we should decide that as a committee.

iSazonov · Nov 24, 2016

I analyzed using NewGuid by @daxian-dbw method
Found 133 *.ps1 scripts on GitHub (I did not search *.psm1).
Thus it is very rarely used cmdlet.

I manually inspected these scripts.
I did not find that this cmdlet was used in loops.
I did not find using Empty guids.

A common usage (in descending order of popularity):

$GUID = (New-Guid).Guid

[string]$Name = (New-Guid).ToString()

$id = New-Guid
$name = "Solution_$id"

$wpconfig = $wpconfig.Replace("eisauthkey",$(New-Guid))

function New-Guid {
    [System.Guid]::NewGuid().ToString("d").Substring(0, 4).Replace("-", "")
}

$guid = (New-Guid).GUID.Replace("-","_")

JamesWTruher · Nov 30, 2016

+        /// <summary>
+        /// returns a guid
+        /// </summary>
+        protected override void EndProcessing()


EndProcessing [](start = 32, length = 13)

this implementation subtly changes the current implementation (the function uses Begin rather than End as is here) - I don't believe it to be a big problem, but thought I should add the note

This is intentional, see this discussion: https://github.com/PowerShell/PowerShell/pull/2786/files/48a6fb2e05c6d4954f94d9f34139ff338aae41a2#r89893087

SteveL-MSFT · Dec 1, 2016

@PowerShell/powershell-committee we discussed this and for discovery reasons as well as shipping this previously, we should have new-guid even though it's a simple operation for users familiar with the guid class

msftclas added the cla-already-signed label Nov 21, 2016

lzybkr suggested changes Nov 21, 2016

View reviewed changes

powercode force-pushed the new-guid-cmdlet branch 2 times, most recently from 5368d9b to 73e2706 Compare November 21, 2016 21:46

PowerShellTeam added the Review - Needed label Nov 21, 2016

powercode force-pushed the new-guid-cmdlet branch from 73e2706 to 065f5e9 Compare November 21, 2016 23:23

vors reviewed Nov 21, 2016

View reviewed changes

powercode force-pushed the new-guid-cmdlet branch from 065f5e9 to b25a1d5 Compare November 22, 2016 00:20

iSazonov reviewed Nov 22, 2016

View reviewed changes

PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 22, 2016

daxian-dbw reviewed Nov 23, 2016

View reviewed changes

daxian-dbw removed the Review - Needed The PR is being reviewed label Nov 23, 2016

lzybkr added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 24, 2016

lzybkr self-assigned this Nov 24, 2016

PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 24, 2016

Moving New-Guid to compiled cmdlet

00ff6eb

powercode force-pushed the new-guid-cmdlet branch from b25a1d5 to 00ff6eb Compare November 24, 2016 19:42

JamesWTruher reviewed Nov 30, 2016

View reviewed changes

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Dec 1, 2016

HemantMahawar removed the Review-Needed label Dec 1, 2016

lzybkr approved these changes Dec 1, 2016

View reviewed changes

lzybkr merged commit 5484b98 into PowerShell:master Dec 1, 2016

powercode deleted the new-guid-cmdlet branch December 6, 2016 21:51

Search code, repositories, users, issues, pull requests...

Conversation

powercode commented Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr commented Nov 24, 2016

Uh oh!

iSazonov commented Nov 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Dec 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

powercode commented Nov 21, 2016 •

edited

Loading

iSazonov Nov 22, 2016 •

edited

Loading

powercode Nov 22, 2016 •

edited

Loading