Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Measure-Object ignore missing properties unless running in strict mode #16589

Merged
merged 4 commits into from Jan 13, 2022

Conversation

KiwiThePoodle
Copy link
Contributor

PR Summary

When using Measure-Object on an empty folder, the "Length" property is invalid. When in StrictMode, an error is thrown when the requested property is not found. The error message is also updated to include the specific property that is not found. The invalid property is simply ignored when not in StrictMode.

PR Context

Fixes issue #14449
Unless in StrictMode, missing properties are typically ignored by other commands, so the Measure-Object command should do the same. This pull-request updates the Measure-Object command to only throw a property not found error when in StrictMode and ignores the property not found when not in StrictMode. This functionality is now consistent with other PowerShell commands.

PR Steps to Reproduce

my_folder is an empty directory

When StrictMode is off: no error
Set-StrictMode -off
gi my_folder | measure Length -sum

When StrictMode is on: error message
Set-StrictMode -Version 3.0
gi my_folder | measure Length -sum

Measure-Object: Cannot process argument because the value of argument “Length” is not valid. Change the value of the “Length” argument and run the operation again.

PR Changed Files

Measure-Object.cs
-Bug fix: changed "Property" to propertyName, the actual name of the property argument
-WritePropertyNotFoundError(propertyName, errorId) only called when in StrictMode

Measure-Object.Tests.ps1
-Added 2 empty folder tests where Measure-Object command is used on an empty folder
-When not in StrictMode, no error should be thrown
-When in StrictMode, error should be thrown

PR Checklist

@ghost
Copy link

ghost commented Dec 10, 2021

CLA assistant check
All CLA requirements met.

$Err[0].FullyQualifiedErrorId | Should -BeExactly "GenericMeasurePropertyNotFound,Microsoft.PowerShell.Commands.MeasureObjectCommand"
}

AfterAll {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be after BeforeAll.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little confused by this suggestion.

Copy link
Collaborator

@iSazonov iSazonov Dec 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all our tests files are formatted so

Describe "Get-Service cmdlet tests" -Tags "CI" {
# Service cmdlet is currently working on windows only
# So skip the tests on non-Windows
BeforeAll {
$originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
if ( -not $IsWindows ) {
$PSDefaultParameterValues["it:skip"] = $true
}
}
# Restore the defaults
AfterAll {
$global:PSDefaultParameterValues = $originalDefaultParameterValues
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, thank you

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 10, 2021
@iSazonov
Copy link
Collaborator

@KiwiThePoodle Please rebase to fix CI-static.

@ghost ghost added the Review - Needed The PR is being reviewed label Dec 21, 2021
@ghost
Copy link

ghost commented Dec 21, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spot this when trying to merge approved PRs on behalf of @TravisEz13, who is OOF this week.
@KiwiThePoodle can you please address this comment?

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Jan 13, 2022
@daxian-dbw daxian-dbw assigned daxian-dbw and unassigned TravisEz13 Jan 13, 2022
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 13, 2022
@pull-request-quantifier-deprecated

This PR has 18 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!


Quantification details

Label      : Extra Small
Size       : +14 -4
Percentile : 7.2%

Total files changed: 2

Change summary by file extension:
.cs : +5 -4
.ps1 : +9 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detetcted.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.


Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@daxian-dbw daxian-dbw merged commit 83a809b into PowerShell:master Jan 13, 2022
@daxian-dbw daxian-dbw changed the title Measure-Object used on an empty folder throws invalid property error only when in StrictMode Make Measure-Object ignore missing properties unless running in strict mode Jan 13, 2022
@daxian-dbw
Copy link
Member

Thanks @KiwiThePoodle for your contribution!

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
@ghost
Copy link

ghost commented Feb 24, 2022

🎉v7.3.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

michaeltlombardi added a commit to michaeltlombardi/PowerShell-Docs that referenced this pull request Mar 8, 2022
This commit adds a note to the `Measure-Object` cmdlet reference
documentation for PowerShell 7.3 to clarify that it now only returns
an argument error when an object is missing the to-measure property
in **StrictMode** to pair with the updated implementation from
PowerShell/PowerShell#16589.
sdwheeler added a commit to MicrosoftDocs/PowerShell-Docs that referenced this pull request Mar 9, 2022
* (GH-8618) Add note to Measure-Object in 7.3

This commit adds a note to the `Measure-Object` cmdlet reference
documentation for PowerShell 7.3 to clarify that it now only returns
an argument error when an object is missing the to-measure property
in **StrictMode** to pair with the updated implementation from
PowerShell/PowerShell#16589.

* Address review feedback.

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Extra Small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants