GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||
GeezerVison is showing github.com — Fix for #6855 - Measure-Object should handle scriptblock properties. by BrucePay · Pull Request #6934 · PowerShell/PowerShell Conversation
|
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
If we use pipeline make sense use some values:
$result = "a,b,c", "d,e" | Measure-Object -Word {$_ -split ','} $result.Words | Should -Be 5
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Maybe move the function and tests in new Context block?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Moved into a separate Describe.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Please add empty new line.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
We should get PowerShell Committee conclusion for new public API. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ilya (@iSazonov) We reviewed this in the last committee meeting and approved the name. I thought Steve Lee (@SteveL-MSFT) had updated the bug to reflect this but I don't see it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Bruce Payette (@BrucePay) Thanks! I am extecting that the PR will marked by the label. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oops - I had the wrong issue number in the comment so you might have looked at the wrong issue. #6855 is, in fact, marked "Committee Reviewed". |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one minor comment.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Could you please use C# 7 syntax?
if (PSObject.Base(target) as Hashtable targetAsHash)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Steve Lee (@SteveL-MSFT) Says we're ignoring CodeFactor issues for now. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ilya (@iSazonov) FYI - Most of my team has been involved with helping port existing Windows PowerShell modules owned by other teams to work with PSCore6 so you may find us not as active. Hopefully we'll be back to fully engaged by end of June (or mostly engaged). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm glad that this process began. I do not see this leading to corrections in PowerShell Core - it convinces me that PowerShell Core is very good! 😄 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
/cc Paul Higinbotham (@PaulHigin) Now that PSPropertyExpression is publically exposed and it has the ability to run scripts, Paul you may want to check if this will introduce any security concerns to the ConstrainedLanguage mode.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This could be a breaking change when it comes to the ConstrainedLanguage mode.
@{prop = 3} | measure-object Count works in ConstrainedLanguage mode today, but with this change, the conversion from Hashtable to PSCustomObject will throw exception.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm - true - good catch. It's pretty obscure and currently the only useful scenario for piping hashtables into measure-object (other than instance-counting). Still, we should probably get a committee review on this.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can you please change the name to WrapHashtableAsPSCustomObject? C# method naming conversion suggests using verbs or verb phrases to name methods.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The point of the rule is that the names of methods should be imperative which this is. There are other examples of (type 1) conditional clauses in .NET such as ThrowIfCancellationRequested on System.Threading.CancellationToken.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
OK. #Closed
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
If we want to handle hashtable specially, shall we expand that to IDictionary? LanguagePrimitives.ConvertPSObjectToType is able to convert IDictionary to PSObject.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The goal is to match the behaviour of [PSCustomObject] @{a=1; b=2}. Currently casting Dictionary<T,T> to PSCustomObject doesn't result in a custom object so I don't think we should introduce that behaviour here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Understood. Good to know that we are matching the behavior of [PSCustomObject] @{a=1; b=2}.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Waiting on the committee's review.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Dongbo Wang (@daxian-dbw) Bruce Payette (@BrucePay) Sorry for the late review. To answer Dongbo's question, from a security point of view I think this is Ok. MshExpression (now PSPropertyExpression) takes a ScriptBlock in its constructor which it will invoke. But as long as the passed in ScriptBlock is created in a constrained environment it will be marked accordingly and the script block will be invoked with CL mode constraints. A constructor also takes a string "expression", but as near as I can tell it is never evaluated as script and is used to look up properties. Is this correct? However, we should include a CL mode test to ensure the expression scriptblock is run in CL mode. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Paul Higinbotham (@PaulHigin) Yes, that's correct. It's for property lookup. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Paul Higinbotham (@PaulHigin) I would like you to take another look at this change. PSPropertyExpression is added to CoreTypes, and that means users can call its methods in ConstrainedLanguage mode. Do you have any concerns about it?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't see that. If this type is going to be white listed then it needs to go through a separate/full security review. I see that some internal methods are now public, and these along with constructors need to be reviewed for vulnerabilities. Please set up a meeting for this.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No - it's only intended to be a type accelerator. I added it in the wrong place. I'll move it to the TypeAccelerators() method where it should be.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree it should be moved :)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't see that. If this type is going to be white listed then it needs to go through a separate/full security review. I see that some internal methods are now public, and these along with constructors need to be reviewed for vulnerabilities. Please set up a meeting for this.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@PowerShell/powershell-committee reviewed this and is fine with this breaking change which is likely only an edge case, but the positive impact for most users is beneficial |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…operties. Fixed by renaming MshExpression to PSPropertyExpression and making it
public. Then in MeasureObjectCommand, lifting it up to the parameter level. Previously the implementation exposed the Property as string and
wrapped it internally as a PSPropertyExpression. Now the parameter type is PSPropertyExpression directly allowing for both wildcard strings
and scriptblocks. PSPropertyExpression now lives in a public namespace where it can be used by cmdlet and script authors to easily add the
same type of functionality to their commands. I also modified PSPropertyExpression to handle hashtables properly as objects so
@{prop = 3} | measure-object prop
and
@{prop = 3} | measure-object {$_.prop}
will work the same. (Previously the example using just the property name would fail.)
Added missing type accelerator test, cleaned up tests.
6ba5c16 to
a7386a9
Compare
|
Paul Higinbotham (@PaulHigin) New commits have been pushed. Can you please update your review? Thanks |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Bruce Payette (@BrucePay) I just noticed that we didn't run feature tests for this PR CI. Can you please push another commit with |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Bruce Payette (@BrucePay) I have submitted a dummy commit to trigger the feature test run for this PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Prior to this change, the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` did not specifically address using those cmdlets against **hashtable** input. In PowerShell 6 an improvement (see PowerShell/PowerShell#6934) enabled users to reference the values of **hashtable** keys in these commands without having to specify calculated properties. This change: - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in versions 7.0 and above to show how users can group, select, and sort **hashtable** objects by their key values - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in Windows PowerShell 5.1 to show how users can group, select, and sort **hashtable** objects with calculated properties - Cleans up Vale linting warnings across those files - Fixes AB#20552 - Resolves MicrosoftDocs#9311
Prior to this change, the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` did not specifically address using those cmdlets against **hashtable** input. In PowerShell 6 an improvement (see PowerShell/PowerShell#6934) enabled users to reference the values of **hashtable** keys in these commands without having to specify calculated properties. This change: - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in versions 7.0 and above to show how users can group, select, and sort **hashtable** objects by their key values - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in Windows PowerShell 5.1 to show how users can group, select, and sort **hashtable** objects with calculated properties - Cleans up Vale linting warnings across those files - Fixes AB#20552 - Resolves MicrosoftDocs#9311
Prior to this change, the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` did not specifically address using those cmdlets against **hashtable** input. In PowerShell 6 an improvement (see PowerShell/PowerShell#6934) enabled users to reference the values of **hashtable** keys in these commands without having to specify calculated properties. This change: - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in versions 7.0 and above to show how users can group, select, and sort **hashtable** objects by their key values - Updates the documentation for `Group-Object`, `Select-Object`, and `Sort-Object` in Windows PowerShell 5.1 to show how users can group, select, and sort **hashtable** objects with calculated properties - Cleans up Vale linting warnings across those files - Fixes AB#20552 - Resolves #9311
…operties. (PowerShell#6934) Measure-Object should handle `ScriptBlock` properties. Fixed by renaming `MshExpression` to `PSPropertyExpression` and making it public. Then in `MeasureObjectCommand`, lifting it up to the parameter level. Previously the implementation exposed the Property as a string and wrapped it internally as a `PSPropertyExpression`. Now the parameter type is `PSPropertyExpression` directly allowing for both wildcard strings and `ScriptBlock`. `PSPropertyExpression` now lives in a public namespace where it can be used by cmdlet and script authors to easily add the same type of functionality to their commands. I also modified `PSPropertyExpression` to handle hashtables properly as objects so @{prop = 3} | measure-object prop and @{prop = 3} | measure-object {$_.prop} will work the same. (Previously the example using just the property name would fail.)
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.