Adding -AllStats Switch parameter for Measure-Object cmdlet #7220
Conversation
Adding -AllStats Switch parameter for Measure-Object cmdlet, Issue #6278
kvprasoon
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
Code factor fixes
Code factor fix
|
@iSazonov I'm not able to understand this Code factor issue. |
| private bool _measureSum; | ||
|
|
||
| /// <summary> | ||
| /// Sets all generic parameters to true and returns all the statitics. |
There was a problem hiding this comment.
Spelling of "statistics".
There was a problem hiding this comment.
oops, that was a typo.
| return; | ||
| } | ||
|
|
||
| if (_allStats) |
There was a problem hiding this comment.
This should go into the BeginProcessing{} block, since it only needs to be run once.
Changes as per review comments.
Code factor fix.
| $testNumbers = 1,1,2,4,5,6 | ||
| $actual = $testNumbers | Measure-Object -AllStats | ||
|
|
||
| $actual.Average | Should -Not -BeNullOrEmpty |
There was a problem hiding this comment.
These Should statements should be testing for numeric values, not string values.
Take a look at the test "Measure-Object with ScriptBlock properties should work". You can use the same range and results for this test.
There was a problem hiding this comment.
@dantraMSFT Thanks, changed the same. But the StandardDeviation is always a decimal value and cannot be asserted with int data type, hence excluded.
There was a problem hiding this comment.
@kvprasoon: I suggest you at least verify it is populated, such as comparing it's ToString() value against the expected string value (2.13697605664328); otherwise, its not completely verified.
| /// <summary> | ||
| /// Does the begin part of the cmdlet. | ||
| /// </summary> | ||
| protected override void BeginProcessing() |
There was a problem hiding this comment.
Actually, you should call the base class implementation, base.BeginProcessing(), here.
Review comment changes
Adding assertion for StandardDeviationin unitMeasure-Object test
|
@kvprasoon Please fix one CodeFactor issue and I'll merge. |
|
@kvprasoon I believe what it is looking for is something like... Gets or sets the value indicating if all statistics should be returned. |
Code factor issue fix.
|
@dantraMSFT Great, it worked. |
|
Reopen the PR to restart CIs. |
|
@iSazonov CI got failed again... |
|
@kvprasoon Thanks for your contribution! |
4 participants
Adding -AllStats Switch parameter for Measure-Object cmdlet, Issue #6278
PR Summary
Adding
-AllStatsparameter forMeasure-Objectcmdlet based on the Issue #6278 .PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.-Allparameter #6278[feature]if the change is significant or affects feature tests