| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks for taking this on, Ilya (@iSazonov). Generally, it sounds we should follow the example of -match and -split (assuming they work consistently among them), which is PSObject.ToStringParser(context, lval), which, from what I gather, is like using "$var" for a non-string value $var. But I don't understand how PSObject.ToStringParser(context, PSObject.AsPSObject(lval)) differs in practice from PSObject.ToStringParser(context, lval). |
Sorry, something went wrong.
In second case we could fall in fast way - TryFastTrackPrimitiveTypes() or IFormattable. In first case a code path is another - follow ETS at first. |
Sorry, something went wrong.
|
I see - but, speaking from observation (haven't looked a the code): Apart from [pscustomobject] instances, ETS properties seem to only affect output formatting, not stringification (in expandable strings). Or are there cases where including ETS properties makes a difference for stringification? If so, what are they, and can you give a specific example? |
Sorry, something went wrong.
|
Michael Klement (@mklement0) The example is in #10389 (comment)
You confuse ETS (Extended Type System) with extensions for Formatting System (Update-TypeData vs Update-FormatData). |
Sorry, something went wrong.
|
I see - you mean an ETS-overridden .ToString() method (I was thinking properties in general, which would only matter in formatting) - that's a helpful example. So it sounds like -join is currently the only string-coercing operator that goes the ETS route. -match, -split, -like do not. In the case of #10389 the ruling was to prevent a behavioral -join change, even if that means forgoing a possible performance optimization. Here, it comes down to this:
I don't know what the performance implications are, but my personal recommendation would be to pick one of the following two options:
|
Sorry, something went wrong.
|
Michael Klement (@mklement0) Great investigations! Thanks! Steve Lee (@SteveL-MSFT) Perhaps PowerShell-Committee should weight these Michael Klement (@mklement0) 's proposed options. |
Sorry, something went wrong.
|
@PowerShell/powershell-committee reviewed this. It's definitely a breaking change (Bruce Payette (@BrucePay) is calling it Bucket 2: Reasonable Gray Area), but we think it's the right thing to do. Given where we're at in the release cycle, and our new snap to .NET's 1-year release cycle, we want to take this as an experimental feature into 7.1-preview.1 so we can understand the breakage and whether we need to revert to old behavior. |
Sorry, something went wrong.
It makes no sense because we can not use strong typed names out of SMA. |
Sorry, something went wrong.
If the experimental feature is not in SMA.dll, then it's not an engine feature and should not be declared in ExperimentalFeature.cs. Instead, it should be declared in the module where that dll is in. |
Sorry, something went wrong.
|
Dongbo Wang (@daxian-dbw) Could you please continue? |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM except for a comment.
Sorry, something went wrong.
|
Should we add CL-Experimental label? |
Sorry, something went wrong.
|
I think we can only use 1 CL-xx label, otherwise the script to generate the change log will be confused. |
Sorry, something went wrong.
It seems Travis said me that CL-Breaking can be used with another CL- label. From releaseTools.psm1 # Array of PRs with multiple labels. The label "CL-BreakingChange" is allowed with some other "CL-*" label. |
Sorry, something went wrong.
|
I don't know about that, but what would you expect the script to do then? placing the same PR under both Breaking Changes and Experimental Features sections? |
Sorry, something went wrong.
|
Hmm, by looking at the following code, it seems the script doesn't handle multiple labels even if CL-breakingchange is one of them, as the switch will fall into the default case when $clLabel contains more than 1 item. PowerShell/tools/releaseTools.psm1 Lines 292 to 313 in 384a7ba |
Sorry, something went wrong.
Here we will have an array and the switch will run for every value from the array. |
Sorry, something went wrong.
|
Ah, you are right, I totally missed that. |
Sorry, something went wrong.
This reverts commit 36d3b3c.
|
🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada: Handy links: |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Fix #10948
PR Context
Options for the fix:
That is before the fix.
Makes lval conversion being culture invariant.
Current fix. Makes lval conversion being culture invariant and add PowerShell magic conversions if lval is PSObject (ex., follow ETS) See
In the case behavior is as for -match operator
The same as previous but always use PowerShell magic conversions (follow ETS).
In the case behavior is as for -join operator (with performance issue because of mandatory wrapping to PSObject)
PR Checklist