-
Notifications
You must be signed in to change notification settings - Fork 8.1k
More value from remaining arguments fixes #5109
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
More value from remaining arguments fixes #5109
Conversation
| // we unwrap our List, but only if there is a single argument of type object[]. | ||
| if (valueFromRemainingArguments.Count == 1 && valueFromRemainingArguments[0] is object[]) | ||
| // we unwrap our List, but only if there is a single argument which is a collection. | ||
| if (valueFromRemainingArguments.Count == 1 && null != LanguagePrimitives.GetEnumerable(valueFromRemainingArguments[0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using LanguagePrimitives.IsTypeEnumerable(Type) for 2 reasons:
This, in fact, shouldn't be a concern as we will do parameter binding using the original object.LanguagePrimitives.GetEnumerableunwrap PSObject, which might strip off ETS properties associated with the PSObject.LanguagePrimitives.GetEnumerableactually calls theGetEnumerabledelegate which may throw an exception or cause a long delay (e.g. fetching object from Azure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
|
Actually, I don't like how that was changed. Sec for another commit. |
|
How about changing |
|
That's part of what I'm doing, but I also want the code from GetEnumerable that checks for PSObject / BaseObject to run. |
|
I added a new LanguagePrimitives.IsObjectEnumerable method to do this, and refactored some duplicated code to a new internal method. Let me know if this is acceptable, since it's making a bit of new public API in LanguagePrimitives. |
| Type objectType = GetBaseObjectType(obj); | ||
| if (objectType == null) { return null; } | ||
| GetEnumerableDelegate getEnumerable = GetOrCalculateEnumerable(objectType); | ||
| return getEnumerable(obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not right. Before the change, the obj passed in getEnumerable is the base object if the original one is PSObject, but now it will be the PSObject in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, you're right. Fixing now.
| /// </param> | ||
| [SuppressMessage("Microsoft.Naming", "CA1720:IdentifiersShouldNotContainTypeNames", MessageId = "obj", Justification = "Since V1 code is already shipped, excluding this message.")] | ||
| public static IEnumerable GetEnumerable(object obj) | ||
| internal static Type GetBaseObjectType(object obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's recommended to always start from private, since currently nothing outside the type depends on it.
|
As for the new public API |
| [SuppressMessage("Microsoft.Naming", "CA1720:IdentifiersShouldNotContainTypeNames", MessageId = "obj", Justification = "Since V1 code is already shipped, excluding this message.")] | ||
| public static IEnumerable GetEnumerable(object obj) | ||
| { | ||
| obj = GetBaseObject(obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about replace GetBaseObject(object) with PSObject.Base(object)? It seems a good fit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just knew there was already going to be a method for this somewhere in the code. :) I searched for .BaseObject and didn't find it; Base() uses the field name instead.
|
I don't think I like that other PR. It's changed Write-Output's parameter from an array type to a single PSObject (which may be undesirable by itself); ValueFromRemainingArguments should really only be used on array parameters, and the change to the parameter binder is to try to shoehorn it into use on a non-array type. |
|
The issue #5122 is interesting, though. I'm not sure whether to consider it a bug or not; we knew that the ValueFromRemainingArguments fix was a breaking change. |
|
I suppose it doesn't matter if the change in #5123 is made to the binder, though. If you assume that ValueFromRemainingArguments should only be used for array type parameters, then it's no harm done; the non-collection argument will be converted back into an array anyway. If ValueFromRemainingArguments should be usable on non-array parameters, then the change in #5123 has value. However, I can't think of any case where that would work other than [object] and [psobject]. Otherwise it's just going to fail when you do pass in multiple arguments to that parameter. I've only used it like "params" in C#, and that only works for arrays. |
|
@dlwyatt So, if array pass thru, when passed as single argument, why |
|
@PowerShell/powershell-committee reviewed this and on the topic of a single element array being a scalar, our decision is that it is simpler for the developer to always expect an array rather than having code handle both an array and a scalar, so it should be an array |
|
Nothing stops a user from applying Actually, the changes in this PR cannot guarantee the |
|
@lzybkr Could you please weigh in? Your thoughts would be helpful. |
|
There are a couple ways to think about this. In C#, it just doesn't make sense for In PowerShell, that's somewhat less of an issue, the pipeline doesn't care, and since V3, many other places handle a single instance an array of objects in a somewhat similar way. On the other hand, we do far too little checking on the use of attributes and if they make sense. I'm all for adding more errors on inappropriate use of an an attribute. One more random thought - not necessarily a good idea, but the only case that truly needs to be a breaking change if we want an error - if the parameter type is explicitly specified and is not |
|
Given the review from powershell committee, I will merge this PR. But please feel free to open an issue and continue the discussion. |
Expands on a previous PR based on conversation here: #2038 (comment)
Unwrapping of ValueFromRemainingArguments was being performed only for object[] arrays (which covers the most common PowerShell binding scenarios), but could be skipped if a collection of any other type were passed to the parameter.