Implicitly convert XML rval to string #2678
Conversation
| @@ -4911,7 +4911,8 @@ protected override object PropertyGet(PSProperty property) | |||
| /// <param name="convertIfPossible">instructs the adapter to convert before setting, if the adapter supports conversion</param> | |||
| protected override void PropertySet(PSProperty property, object setValue, bool convertIfPossible) | |||
| { | |||
| string valueString = setValue as string; | |||
| // XML is always a string so implicitly convert to string | |||
| string valueString = setValue?.ToString(); | |||
There was a problem hiding this comment.
Calling ToString directly avoids any magic PowerShell might apply when converting a value to string.
You could just call LanguagePrimitives.ConvertTo<string>(setValue) - this will properly call into a CodeMethod or ScriptMethod named ToString if the object has one.
There was a problem hiding this comment.
Done.
| # rval will be implicitly converted to `string` type | ||
| { $x.root.data = $rval } | Should Not Throw | ||
| $x.root.data | Should Be $rval.ToString() | ||
| } |
There was a problem hiding this comment.
You should also test that we properly invoke ToString implemented as a CodeMethod and ScriptMethod. System.Type has a CodeMethod implementation of ToString, you can see the difference:
#57 PS> [System.Int32].ToString()
System.Int32
#58 PS> [string][System.Int32]
int
There was a problem hiding this comment.
Done.
Only I could not create ToString ScriptMethod because ToString already exists.
d6f72d2 to
3ffe8fb
Compare
|
@lzybkr could you continue the code review? |
| @@ -4911,7 +4911,8 @@ protected override object PropertyGet(PSProperty property) | |||
| /// <param name="convertIfPossible">instructs the adapter to convert before setting, if the adapter supports conversion</param> | |||
| protected override void PropertySet(PSProperty property, object setValue, bool convertIfPossible) | |||
| { | |||
| string valueString = setValue as string; | |||
| // XML is always a string so implicitly convert to string | |||
| string valueString = LanguagePrimitives.ConvertTo<string>(setValue); | |||
| if (valueString == null) | |||
There was a problem hiding this comment.
I think this condition is unreachable now.
We can convert everything to string (including $null which turns into the empty string).
The only failure condition now is if the conversion to string fails with an exception, e.g. if ToString() throws an exception. This should be rare, but if it happens, I think we'd want to see that exception and not the SetValueException that is being thrown here.
In other words, I think this test and subsequent throw can be safely deleted.
| @{ rval = @{testprop = 1}; value = 'hash (psobject)' }, | ||
| @{ rval = 1; value = 'int (codemethod)' }, | ||
| @{ rval = "teststring"; value = 'string (codemethod)' } | ||
| } |
There was a problem hiding this comment.
I would add tests for:
- $null
- array of strings
- array of integers
- psobject wrapping each of the previous values
There was a problem hiding this comment.
Fixed.
But I have doubts about [PSObject]::AsPSObject($null) and [PSObject]::AsPSObject(1)
| param($rval) | ||
| { | ||
| { $x.root.data = $rval } | Should Not Throw | ||
| $x.root.data | Should Be $rval.ToString() |
There was a problem hiding this comment.
We don't always call ToString.
PS> $x = 1,2
PS> [System.Management.Automation.LanguagePrimitives]::ConvertTo($x, [string])
1 2
PS> $x.ToString()
System.Object[]1. Implicitly convert XML rval to string 2. Add test
a0c98ba to
a1a94ba
Compare
|
@lzybkr Do you have any other comments or concerns regarding this PR? |
|
@lzybkr Thanks for code review and great comments! |
Close #2459