MeasureInfo: Only display properties with values #6214

powercode · 2018-02-21T22:23:02Z

PR Summary

Adding Item selection conditions to the list formatting info for MeasureInfo so that only the properties with values gets displayed

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing Documentation needed
- Issue filed - Issue link:
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affects feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…ition

iSazonov · 2018-02-22T14:23:44Z

test/powershell/Modules/Microsoft.PowerShell.Utility/object.tests.ps1

+
+            $text = 1, 2, 3 | Measure-Object -Minimum -Maximum | format-list | out-string
+            $text -match "min|max" | should -BeTrue
+        }


Please use a correct letter case in all lines.

iSazonov · 2018-02-22T14:25:29Z

...ystem.Management.Automation/FormatAndOutput/common/DisplayDatabase/displayDescriptionData.cs

        }

+        /// <summary>
+        /// Creates a DisplayEntry for the given scriptblock


Please add final dot in public comments.

iSazonov · 2018-02-22T14:26:15Z

...ystem.Management.Automation/FormatAndOutput/common/DisplayDatabase/displayDescriptionData.cs

+        /// Creates a DisplayEntry for the given scriptblock
+        /// </summary>
+        /// <param name="scriptblock">The content of the scriptblock</param>
+        public static DisplayEntry CreateScriptBlockEntry(string scriptblock)


Where we use the method?

We don't.
But CreatePropertyEntry is used now, so I added the scriptblock one for completeness.

Clear.
We should add tests for the public method.

iSazonov · 2018-02-22T14:27:27Z

....Management.Automation/FormatAndOutput/common/DisplayDatabase/displayDescriptionData_List.cs


        /// <summary></summary>
-        public ListEntryBuilder AddItemScriptBlock(string scriptBlock, string label = null, string format = null)
+        public ListEntryBuilder AddItemScriptBlock(string scriptBlock, string label = null, string format = null, DisplayEntry itemSelectionContition = null )


Is the change binary backward compatible? We shouldn't break public API.

The change is not binary compatible.

That said, it is unlikely anyone uses this api yet, people still rely on a ps1xml file because there isn't a good way for a module author to use this api in a useful way yet.

iSazonov · 2018-02-22T14:30:05Z

....Management.Automation/FormatAndOutput/common/DisplayDatabase/displayDescriptionData_List.cs


        /// <summary></summary>
-        public ListEntryBuilder AddItemProperty(string property, string label = null, string format = null)
+        public ListEntryBuilder AddItemProperty(string property, string label = null, string format = null, DisplayEntry itemSelectionContition = null)


iSazonov · 2018-02-22T14:36:05Z

...stem.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs

-                        .AddItemProperty(@"Maximum")
-                        .AddItemProperty(@"Minimum")
-                        .AddItemProperty(@"Property")
+                        .AddItemProperty(@"Average",  itemSelectionContition: DisplayEntry.CreatePropertyEntry("Average"))


We could add AddItemPropertyByCondition.

iSazonov · 2018-02-22T18:45:35Z

@SteveL-MSFT The PR contains a public API change - PowerShell Committee conclusion needed.

iSazonov · 2018-02-25T12:18:58Z

test/powershell/Modules/Microsoft.PowerShell.Utility/object.tests.ps1

        It 'should only display fields that are set' {
-            $text = 1, 2, 3 | Measure-Object -sum -Average | format-list | out-string
-            $text -match "min|max" | should -BeFalse
+            $text = 1, 2, 3 | Measure-Object -sum -Average | Format-List | Out-String


Please replace sum with Sum.

iSazonov · 2018-02-25T17:20:50Z

@powercode Thanks! We are waiting PowerShell Committee.

JamesWTruher · 2018-02-28T23:23:25Z

test/powershell/Modules/Microsoft.PowerShell.Utility/object.tests.ps1

+
+        It 'should only display fields that are set' {
+            $text = 1, 2, 3 | Measure-Object -Sum -Average | Format-List | Out-String
+            $text -match "min|max" | Should -BeFalse


would it be better if this was a positive test? What should this actually look like? or both positive/negative?

SteveL-MSFT · 2018-02-28T23:49:14Z

@PowerShell/powershell-committee reviewed this and is ok with the public apis

stale · 2018-04-20T21:09:49Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

stale · 2018-04-30T22:06:17Z

This PR has be automatically close because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it.
Thanks again for your contribution.
Community members are welcome to grab these works.

powercode · 2018-06-18T21:22:08Z

Updating

Staffan Gustafsson and others added 3 commits February 21, 2018 22:58

Adding support for built-in List formats to include ItemSelectionCond…

1251614

…ition

Adding ItemSelectionCondition to GenericMeasureInfo

b85ee51

Adding tests for MeasureInfo format-list

4dbc438

iSazonov reviewed Feb 22, 2018

View reviewed changes

iSazonov assigned daxian-dbw, lzybkr, SteveL-MSFT and iSazonov and unassigned daxian-dbw, lzybkr and SteveL-MSFT Feb 22, 2018

iSazonov requested review from SteveL-MSFT, daxian-dbw and lzybkr February 22, 2018 14:37

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 22, 2018

Addressing review comments

1eb5725

iSazonov reviewed Feb 25, 2018

View reviewed changes

fixing casing of Sum

6de3044

JamesWTruher reviewed Feb 28, 2018

View reviewed changes

iSazonov 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 Mar 1, 2018

stale bot added the Stale label Apr 20, 2018

stale bot closed this Apr 30, 2018

powercode changed the title ~~MeasureInfo: Only display properties with values~~ WIP MeasureInfo: Only display properties with values Jun 18, 2018

powercode changed the title ~~WIP MeasureInfo: Only display properties with values~~ MeasureInfo: Only display properties with values Jun 18, 2018

powercode mentioned this pull request Jun 18, 2018

Only display properties with values for MeasureInfo #7104

Merged

11 tasks

MeasureInfo: Only display properties with values #6214

MeasureInfo: Only display properties with values #6214

Uh oh!

Conversation

powercode commented Feb 21, 2018 • edited by SteveL-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

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 commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Feb 25, 2018

Uh oh!

JamesWTruher Feb 28, 2018 • 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!

SteveL-MSFT commented Feb 28, 2018

Uh oh!

stale bot commented Apr 20, 2018

Uh oh!

stale bot commented Apr 30, 2018

Uh oh!

powercode commented Jun 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

powercode commented Feb 21, 2018 •

edited by SteveL-MSFT

Loading

JamesWTruher Feb 28, 2018 •

edited

Loading