Fix Select-Object to approve UX and fix return a property named * by iSazonov · Pull Request #2421 · PowerShell/PowerShell

iSazonov · 2016-10-04T07:26:57Z

Now ExcludeProperty imply Property = '*' to approve UX Select-Object -ExcludeProperty dependency on -Property is unintuitive. #2351
Don't return property named '*' Select-Object return a property named * #2420

msftclas · 2016-10-04T07:27:00Z

Hi @iSazonov, 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;

1. Now ExcludeProperty imply Property = '*' 2. Don't return property named '*'

iSazonov · 2016-10-06T08:28:30Z

@PowerShellTeam please review the code.

daxian-dbw · 2016-10-07T23:07:12Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/select-object.cs

-            if (expressionResults.Count == 0)
+            if (expressionResults.Count == 0 && !ex.HasWildCardCharacters)
            {
                //Commented out for bug 1107600


Could you please remove the commented-out code snippet here? It's useless and there is no way to figure out what bug 1107600 is now :)

And it would be better if you can add some comments here, explaining your purpose (allow Select-Object -Property noexist-name to return a PSObject with property noexist-name, unless noexist-name itself contains wildcards).
I find the implementation of Select-Object is so poorly commented (near to zero comment) that it becomes a pain to read the code. So let's add some when we make changes in this file.

I generally see little comments throughout the Powershell code and tests and thought it was best practice. I will make more comments.
I think @PowerShellTeam needs to mark this moment in documentation: "make more comments in your code".

I think @PowerShellTeam needs to mark this moment in documentation: "make more comments in your code".

I absolutely agree! I will update coding-conventions to include this.

daxian-dbw · 2016-10-07T23:22:22Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+        # Issue #2420
+	It "Select-Object with implicit Property = '*' don't return property named '*'"{
+		$results = [pscustomobject]@{Thing="thing1"} | Select-Object -ExcludeProperty thing 
+		$results.psobject.Properties.Item("*").Name | Should Be $null


Item("*").Name

.Name seems not necessary here.

daxian-dbw · 2016-10-07T23:39:49Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/select-object.cs

            {
                _exclusionFilter = new MshExpressionFilter(ExcludeProperty);
+                if ((Property == null) || (Property.Length == 0))
+                {


This will result a breaking change -- the scirpt below runs successfully before this change; but with this change a bunch errors will be thrown out.

$s = Get-Process -Id $pid | Select-Object -ExcludeProperty ProcessName -ExpandProperty Modules

However, I don't feel this would break any existing scripts practically. It's documented that -ExcludeProperty has no effect without -Property, so I don't think any existing script would only specify -ExcludeProperty.

Maybe we can analyze the powershell scripts on GitHub to get some data. We can borrow @lzybkr's AnalyzeCurlUsage.ps1.

1.The current tests do not catch it. I think we should add this in tests.

2.There is no difference between:

$s = Get-Process -Id $pid | Select-Object -ExpandProperty Modules $s[0] | fl *

and

$s = Get-Process -Id $pid | Select-Object -ExcludeProperty FileName -ExpandProperty Modules $s[0] | fl *

i.e. ExpandProperty skip ExcludeProperty. So the change could be corrected without breaking.

3.The cmdlet is very intuitive to the user UX and very controversial:
ExcludeProperty is skipped w/o Property
ExpandProperty skip ExcludeProperty
ExpandProperty with Property throw exception
Compare:

1 | Select-Object "1" | Select-Object 1 | Select-Object -Property * "1" | Select-Object -Property *

and many more.
And it is a tricky code, and any even slightly change this code can easily break down.

(In general I think this cmdlet should be replaced with a newer, which meets modern requirements to cmdlets.)

I think you mean that it's very unintuitive 😕

If the intention is (to maintain the current expected behavior) that -ExpandProperty ignores -ExcludeProperty wouldn't it be better to have these switches in separate parameter sets? I'd certainly rather have it be obvious that they can't be used together than to allow them together but ignore one (or worse, produce runtime errors if they're used together)...

I think you mean that it's very unintuitive 😕

Under "intuitive" I mean that we use the cmdlet 10 years "as is" without many complaints (In other words, users get results they expect).
All completely different if you look on the inner side of the cmdlet. I guess it was one of the first cmdlets of Powershell. Today such a design would be immediately rejected. The code is tricky and any chip change results in multiple test crush.
And we absolutely cannot add parameter sets (and moreover we cannot add new exceptions) because this is breaking change.
As I said above, time to replace this cmdlet to a new one with stringent modern design, better UX and better performance.

@daxian-dbw I fix breaking change and add your test.

The conclusion is to take your changes to ProcessExpressionParameter() in your second commit -- always add "*" to -Property when -ExcludeProperty is specified but -Property is not : 70c3038

I think you implemented the pseudo code I listed above to avoid the breaking change, but as we discussed here, that will raise new confusion about how -ExlcudeProperty works. So I believe it's reasonable to take your original changes to ProcessExpressionParameter() to make it simple for users to understand.

Note that I hadn't actually concluded anything after my discussion w/ @daxian-dbw, but after some of my own experiments, I do agree. The existing behavior is surprising and not useful, anyone potentially hit by the breaking change wasn't getting what they wanted anyway.

@daxian-dbw @lzybkr Thanks for the clarification and the final conclusion! I'll make the rollback.

Thank you @iSazonov
#Closed

daxian-dbw · 2016-10-07T23:41:11Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+        # Issue #2420
+	It "Select-Object with explicit Property = '*' don't return property named '*'"{
+		$results = [pscustomobject]@{Thing="thing1"} | Select-Object -Property * -ExcludeProperty thing 
+		$results.psobject.Properties.Item("*").Name | Should Be $null


.Name seems not necessary.

daxian-dbw · 2016-10-07T23:43:15Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+        # Issue #2351
+	It "Select-Object with implicit Property = '*' exclude not single prperty"{
+		$results = [pscustomobject]@{Thing="thing1";Param2="param2"} | Select-Object -ExcludeProperty Param2 
+                $results.Param2 | Should Be $null


Please fix the indentation here.

daxian-dbw · 2016-10-07T23:43:24Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+        # Issue #2351
+	It "Select-Object with explicit Property = '*' exclude not single prperty"{
+		$results = [pscustomobject]@{Thing="thing1";Param2="param2"} | Select-Object -Property * -ExcludeProperty Param2 
+                $results.Param2 | Should Be $null


Please fix the indentation here.

RichardSiddaway · 2016-10-09T20:01:38Z

Having separate parameter sets would be a great idea

Sent from my iPad

On 9 Oct 2016, at 20:30, Joel Bennett <[email protected]mailto:[email protected]> wrote:

@Jaykul commented on this pull request.

In src/Microsoft.PowerShell.Commands.Utility/commands/utility/select-object.cshttps://github.com//pull/2421:

@@ -337,6 +337,11 @@ private void ProcessExpressionParameter()
if (ExcludeProperty != null)
{
_exclusionFilter = new MshExpressionFilter(ExcludeProperty);
           if ((Property == null) || (Property.Length == 0))
           {

I think you mean that it's very unintuitive ?

If the intention is (to maintain the current expected behavior) that -ExpandProperty ignores -ExcludeProperty wouldn't it be better to have these switches in separate parameter sets? I'd certainly rather have it be obvious that they can't be used together than to allow them together but ignore one (or worse, produce runtime errors if they're used together)...

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/2421, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AHgYUaGr7eou2GSu_7Q0m2-iosO0xvc0ks5qyUDjgaJpZM4KNXvs.

Restore ExpandProperty skip ExcludeProperty Add new test Add new comments Remove old comment

iSazonov · 2016-10-10T06:39:18Z

@RichardSiddaway Sorry, this is the right decision but it is impossible to add parameter sets w/o breaking change. See my comment above.

daxian-dbw · 2016-10-10T21:08:26Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/select-object.cs


-            if (ExcludeProperty != null)
+            // ExpandProperty skip processing ExcludeProperty
+            if (ExcludeProperty != null && string.IsNullOrEmpty(ExpandProperty))


We cannot ignore -ExcludeProperty just because -ExpandProperty is specified. See the script below:

PS:88> $s = Get-Process -Id $pid | Select-Object -Property Process* -ExpandProperty Modules PS:89> $s[0].ProcessName powershell PS:90> $s[0].ProcessorAffinity 255 PS:91> $p = Get-Process -Id $pid | Select-Object -Property Process* -ExcludeProperty ProcessorAffinity -ExpandProperty Modules PS:92> $p[0].ProcessName powershell PS:93> $p[0].ProcessorAffinity PS:94>

-ExcludeProperty took effect and excluded the property ProcessorAffinity, so only ProcessName got added to each Module object.

I feel it's OK to break the Select-Object -ExcludeProperty value1 -ExpandProperty value2 usage based on my analysis of the usage of Select-Object in GitHub (please see my comment above).

Ok, waiting team approve above then revert the change.

While waiting I made refactoring. See comment above.

daxian-dbw · 2016-10-10T23:08:12Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/select-object.cs

                }
            }

+            // allow Select-Object -Property noexist-name to return a PSObject with property noexist-name,


It would be clearer if you add quotes around Select-Object -Property noexist-name.

1. Refactoring ProcessExpressionParameter() to make explicit logic for processing Property, ExcludeProperty and ExpandProperty 2. Add new test 3. Correct comment

1. Revert ProcessExpressionParameter() to second commit 2. Remove unneeded test

daxian-dbw · 2016-10-31T17:30:07Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+	}
+
+        # Issue #2351
+	It "Select-Object with implicit Property = '*' exclude not single prperty"{


prperty

A typo, should be 'property'

daxian-dbw · 2016-10-31T17:30:43Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+	}
+
+        # Issue #2351
+	It "Select-Object with explicit Property = '*' exclude not single prperty"{


prperty

Same typo :)

It's fate :)
Fixed.

daxian-dbw · 2016-10-31T17:33:54Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

+		$p = Get-Process -Id $pid | Select-Object -Property Process* -ExcludeProperty ProcessorAffinity -ExpandProperty Modules
+		$p[0].psobject.Properties.Item("ProcessorAffinity") | Should Be $null
+    }
+}


Can you please add a new line?

Thanks. #Closed

daxian-dbw · 2016-10-31T17:37:32Z

Thanks @iSazonov for the updates! Once the typos in tests are fixed, the PR will be ready to merge.

daxian-dbw · 2016-11-01T05:47:24Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/select-object.cs

+                if ((Property == null) || (Property.Length == 0))
+                 {
+                    Property = new Object[]{"*"};
+                    _propertyMshParameterList = processor.ProcessParameters(Property, invocationContext);                }


@iSazonov sorry that I missed this one this morning. Can you please fix the indentation of the opening '{' and move the closing '}' to a new line? And please also remove the extra newline at line#348.

daxian-dbw

LGTM.

iSazonov · 2016-11-02T11:08:57Z

@daxian-dbw Thanks very much!

daxian-dbw · 2016-11-02T16:32:34Z

@iSazonov Thank you for your patience and the great contribution 👍

msftclas added the cla-already-signed label Oct 4, 2016

iSazonov added 2 commits October 5, 2016 17:04

Add test for Select-Object with Property = '*'

42fe5b8

Fix Select-Object with Property = '*'

70c3038

1. Now ExcludeProperty imply Property = '*' 2. Don't return property named '*'

iSazonov force-pushed the selectobject branch from 3e9cef1 to 70c3038 Compare October 5, 2016 14:06

daxian-dbw self-assigned this Oct 6, 2016

daxian-dbw added the Review - Needed label Oct 6, 2016

daxian-dbw requested changes Oct 7, 2016

View reviewed changes

iSazonov mentioned this pull request Oct 8, 2016

Added -Top and -Bottom aliases for -First and -Last #2446

Closed

Fixes after code review

7b09e58

Restore ExpandProperty skip ExcludeProperty Add new test Add new comments Remove old comment

daxian-dbw reviewed Oct 10, 2016

View reviewed changes

Refactoring ProcessExpressionParameter()

e5f6d90

1. Refactoring ProcessExpressionParameter() to make explicit logic for processing Property, ExcludeProperty and ExpandProperty 2. Add new test 3. Correct comment

iSazonov force-pushed the selectobject branch from b34111c to e5f6d90 Compare October 12, 2016 09:32

Revert ProcessExpressionParameter() to second commit

5492462

1. Revert ProcessExpressionParameter() to second commit 2. Remove unneeded test

daxian-dbw reviewed Oct 31, 2016

View reviewed changes

Fix typo in test

cf8654a

iSazonov force-pushed the selectobject branch from 1d06ff1 to cf8654a Compare November 1, 2016 03:07

daxian-dbw reviewed Nov 1, 2016

View reviewed changes

Fix indentation

96f2777

daxian-dbw approved these changes Nov 1, 2016

View reviewed changes

daxian-dbw merged commit 5aee4ba into PowerShell:master Nov 1, 2016

iSazonov deleted the selectobject branch November 2, 2016 11:09

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Jun 17, 2017

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for ' Fix Select-Object to approve UX and fix return a property named *' MicrosoftDocs/PowerShell-Docs#3113

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Conversation

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

Uh oh!

Uh oh!

msftclas commented Oct 4, 2016

Uh oh!

iSazonov commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 7, 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!

iSazonov Oct 8, 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!

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!

daxian-dbw Oct 7, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardSiddaway commented Oct 9, 2016

Uh oh!

iSazonov commented Oct 10, 2016

Uh oh!

daxian-dbw Oct 10, 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!

iSazonov commented Oct 4, 2016 •

edited

Loading

daxian-dbw Oct 7, 2016 •

edited

Loading

iSazonov Oct 8, 2016 •

edited

Loading

daxian-dbw Oct 7, 2016 •

edited

Loading

daxian-dbw Oct 10, 2016 •

edited

Loading