Fixing HashMap to PSCustomObject object conversion (#2295) by charub · Pull Request #2533 · PowerShell/PowerShell

charub · Oct 25, 2016

Fixing following issues :

HashMap to PSCustomObject Conversion fails if full name of PSCustomObject type is specified.
[pscu] expands to [System.Management.Automation.PSCustomObject] whereas [pso] expands to [PSObject].
[PSCustomObject].FullName returned System.Management.Automation.PSObject instead of System.Management.Automation.PSCustomObject.

After the fix, these 3 should not happen.

msftclas · Oct 25, 2016

Hi @charub, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Charu Bassi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

vors · Oct 26, 2016

@charub it fixes #2295 right?

vors

I'd also love to see more tests:

ctor call with new() on type accelerator, on full type name
ctor call with New-Object with full name, short name
hashtable cast to full name

vors · Oct 26, 2016

Not sure about that: System.Management.Automation.PSCustomObject is a different type from System.Management.Automation.PSObject which we construct. Did you have a chance to figure out what's the difference between them?

Don't need this change anymore. Only fixing the tab completion for 'pscu'

vors · Oct 26, 2016

We should avoid duplicated code, consider create a helper function.

Don't need this change anymore. Only fixing the tab completion for 'pscu'

vors · Oct 26, 2016

I could be wrong, but it seems inconsistent with the current proposed conversion: the acelerator is for type PSCustomObject, but binder for hashtable does the conversion to PSObject. One option would be to completely eliminate PSCustomObject and set type accelerator to PSObject, if that doesn't break anything.

This is a breaking change. I realize it feels wrong, but it's been this way since V1, we can't really change it.

Don't need this change anymore.

lzybkr

I do question why anybody would want to use [System.Management.Automation.PSCustomObject]@{}.

If it only comes up because of tab completion, maybe we should special case tab completion of [pscustomobject].

lzybkr · Oct 26, 2016

This is a breaking change. I realize it feels wrong, but it's been this way since V1, we can't really change it.

charub · Nov 4, 2016

It seems the problem is the tab completion for pscu. 'PSCU' expands to [System.Management.Automation.PSCustomObject] when it should actually expand to [pscustomobject] which is an accelerator for System.Management.Automation.PSObject.
I agree with Jason that we should fix the tab completion.

vors · Nov 12, 2016

Ok, that sounds reasonable: if we just fix IntelliSense then we will probably avoid most of the problems.
Current fix seems too much of a special case for me. It's hard to track down that this code affects IntelliSense and nothing else.
I also noticed that completion for [pscu in Emacs PSReadLine mode (default on Unix) is completely screwed.
It turn into [PS instead of anything meanfull.
It could be a PSReadLine issue (not the fitst time I see problems with Emacs mode).

lzybkr · Nov 14, 2016

This change seems fine (and necessary) to me, but perhaps it's not sufficient to fix the ambiguity that comes up in PSReadline.

We definitely want some tests.

It does fix more than Intellisense, e.g. if a method used PSCustomObject in it's signature, this change will affect the output of Get-Member.

vors · Nov 14, 2016

@charub please add some tests and we will merge it.

vors · Nov 17, 2016

I would argue about correctness of this test.
It's a current behavior, but it's not the right one.
Maybe just drop the check for count and check for [1] == psobject and leave a comment explaining what's going on?

As per the current behavior, pscu expands to System.Management.Automation.PSCustomObject. After this change, it will now expand to pscustomobject and psobject. both of these accelerators refer to System.Management.Automation.PSObject. I am afraid I don't understand your concern here.

Is expanding to both psobject and pscustomobject a correct behavior for [pscu?
I don't think so. I think it should be pscustomobject only, because psobject doesn't start with pscu.

Why then make a test that verifies a wrong behavior?

I think it's the correct behavior considering both are accelerators for [System.Management.Automation.PSObject].

I cannot agree with that.

vors · Nov 17, 2016

empty hashtable or $null

This test is only for $null
Can you one more testcase for empty hashtable?

Added another test for an empty hashtable

vors · Nov 17, 2016

fail to create object

Any object? -Property $null seems like a special case.

Again the same question: is it the right test or it's just the current behavior.

you are right. Corrected the test.

vors · Nov 21, 2016

I'm currently waiting on the $result.CompletionMatches[1].CompletionText | Should Be "psobject" thread. Lets find solution and then we can merge the PR.

charub · Dec 9, 2016

@vors : You are right, pscu should only expand to pscustomobject and not psobject. Fixed the current behavior for tab completion when a type has more than 1 accelerators.

Resolving PowerShell#2295 Fixing the current behavior of tab completion for types with more than 1 type accelerator.

vors · Dec 21, 2016

        }

-        internal static string FindBuiltinAccelerator(Type type)
+        internal static string FindBuiltinAccelerator(Type type,string expectedKey=null)


Style:

space after comma,

spaces around =

vors · Dec 21, 2016

+            $errorObj = $_
+        }
+        $obj | should be $null
+        $errorObj.FullyQualifiedErrorId | should be "CannotFindAppropriateCtor,Microsoft.PowerShell.Commands.NewObjectCommand"


Should we add a follow-up item on that to completely remove this type?
cc @lzybkr

It's probably not worth the hassle - it's a public api and hence a breaking change, and there is one valid use of the type (though that use can be expressed differently).

vors

Please, correct the style comments and it will be good for merge

Type completion would behave strangely when there were multiple aliases for a given type (currently only possible with type accelerators). The fix is to only select the accelerator that matches what the user typed. Closes PowerShell#2295

msftclas added the cla-not-required label Oct 25, 2016

mirichmo added the Review - Needed label Oct 25, 2016

vors self-assigned this Oct 26, 2016

vors suggested changes Oct 26, 2016

View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed labels Oct 26, 2016

lzybkr reviewed Oct 26, 2016

View reviewed changes

charub force-pushed the hashmap-to-pscustomobject-conversion-fix branch from 65527df to 677bc79 Compare November 4, 2016 21:01

PowerShellTeam added Review - Needed and removed Review - Waiting on Author labels Nov 10, 2016

vors added Review - Waiting on Author and removed Review - Needed labels Nov 14, 2016

charub force-pushed the hashmap-to-pscustomobject-conversion-fix branch from 677bc79 to 374fd88 Compare November 16, 2016 00:07

vors removed the Review - Waiting on Author label Nov 17, 2016

lzybkr added the Review - Needed label Nov 17, 2016

vors suggested changes Nov 17, 2016

View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed labels Nov 17, 2016

PowerShellTeam added Review - Needed and removed Review - Waiting on Author labels Nov 19, 2016

charub force-pushed the hashmap-to-pscustomobject-conversion-fix branch from 374fd88 to 01cf2c4 Compare November 19, 2016 01:00

vors added Review - Waiting on Author and removed Review - Needed labels Nov 19, 2016

PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 5, 2016

charub force-pushed the hashmap-to-pscustomobject-conversion-fix branch from 01cf2c4 to 124114e Compare December 9, 2016 20:21

Fixing pscu tab completion to psustomobject (PowerShell#2295)

ed6a8ae

Resolving PowerShell#2295 Fixing the current behavior of tab completion for types with more than 1 type accelerator.

charub force-pushed the hashmap-to-pscustomobject-conversion-fix branch from 124114e to ed6a8ae Compare December 9, 2016 23:19

vors reviewed Dec 21, 2016

View reviewed changes

vors approved these changes Dec 21, 2016

View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 21, 2016

Fix code formatting

58a7df4

lzybkr approved these changes Dec 21, 2016

View reviewed changes

lzybkr merged commit 821c965 into PowerShell:master Dec 21, 2016

TravisEz13 added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Waiting on Author labels May 27, 2020

Search code, repositories, users, issues, pull requests...

Conversation

charub commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Oct 25, 2016

Uh oh!

vors commented Oct 26, 2016

Uh oh!

vors left a comment

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!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charub commented Nov 4, 2016

Uh oh!

vors commented Nov 12, 2016

Uh oh!

lzybkr commented Nov 14, 2016

Uh oh!

vors commented Nov 14, 2016

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!

vors commented Nov 21, 2016

Uh oh!

charub commented Dec 9, 2016

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!

vors left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

charub commented Oct 25, 2016 •

edited

Loading