Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAllow explicitly specified named parameter to supersede the same one from hashtable splatting #13162
Conversation
I have a concern about this statement. What if such Hashtable is a config with all possible parameters and an user sends the config to functions? |
For cmdlet/advanced functions, this will fail. |
|
@BrucePay would love your opinion on this behavior |
|
@daxian-dbw you said: "so |
|
@bpayette Right, I just corrected that comment. Thanks! |
|
@PowerShell/powershell-committee discussed this and would like more community feedback. The disagreement was on whether the splatted hashtable order matters. In one case, the order dictates who overrides: $hash = @{ Name = 'hello' }
test-function @hash -Name World
test-function -Name World @hashIf the ordering matters, then in the first case you get The argument for ordering is that it's been a useful construct in bash where latter parameters win so you can use it to override parameters declared in aliases. The argument against is that ordering (ignoring positional parameters) has never mattered in PowerShell so this is introducing a new concept and may confuse users. |
|
Ordering shouldn't matter. As @daxian-dbw it simply increases fragility and massively decreases readability if order matters for this. With the knowledge that order doesn't matter, I can confidently state that If order mattered, you would have to always keep track of if the parameter is before or after, and if it's before the hashtable you then have to look up whether the param you're worried about is in the hashtable. Way less obvious what's going on. Also, the hashtable is much more likely to be the reused element there, not the individual command call statement. I think it stands to reason that the more deliberate statement (assuming reuse of hashtable means that the params it contains are probably more general than just the one command call) with the lesser scope of effect should take precedence. |
I agree. It seems it is not PowerShell native behavior to depend on order of named parameters. |
|
Also if we consider |
|
It seems to me that this behavior of overriding a splat is a natural follow-on from |
This argument would extend that parameter behavior should be this way in all cases; so The explicit parameter should override anything splatted. It's clearer, and more natural. As a personal preference, I generally splat at the end. Parameters and variables get long, so hitting the @ means I'm done with the line and know what's happening. With order-matters, I (and presumably others) would have to alter their coding practice to leverage this. That's the breaking change with order-matters. If you splat first today, there's no change in behavior/code; if you splat late, it does. |
Agreed. |
|
@PowerShell/powershell-committee reviewed this, we agree that within PowerShell ordering has not mattered, so we should continue that. On the breaking change for SimpleFunctions, parameters have always been implicitly positional, so the new change is accepted. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
src/System.Management.Automation/engine/hostifaces/Parameter.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CmdletParameterBinderController.cs
Outdated
Show resolved
Hide resolved
msftbot
bot
added
Waiting on Author
and removed
Review - Needed
Waiting on Author
labels
|
LGTM |
| /// and then bind the argument to the parameter. | ||
| /// </summary> | ||
| /// <param name="parameterSets"> | ||
| /// The parameter set used to bind the arguments. | ||
| /// </param> | ||
| /// <param name="arguments"> | ||
| /// The arguments that should be attempted to bind to the parameters of the specified | ||
| /// parameter binder. | ||
| /// </param> | ||
| /// <exception cref="ParameterBindingException"> | ||
| /// if multiple parameters are found matching the name. | ||
| /// or | ||
| /// if no match could be found. | ||
| /// or | ||
| /// If argument transformation fails. | ||
| /// or | ||
| /// The argument could not be coerced to the appropriate type for the parameter. | ||
| /// or | ||
| /// The parameter argument transformation, prerequisite, or validation failed. | ||
| /// or | ||
| /// If the binding to the parameter fails. | ||
| /// </exception> | ||
| private Collection<CommandParameterInternal> BindParameters(uint parameterSets, Collection<CommandParameterInternal> arguments) | ||
| protected override void BindNamedParameter( |
iSazonov
Aug 5, 2020
Collaborator
(As side note, there is a full mess with 'bind the argument' vs 'bind the parameter' in comments and method names.)
daxian-dbw
Aug 5, 2020
Author
Member
When talking about parameter binding, it should be bind an argument to a parameter, an argument is bound (to a parameter), a parameter is bound (by an argument).
iSazonov
added
CL-BreakingChange
CL-Engine
Documentation Needed
and removed
Documentation Needed
labels
iSazonov
merged commit 63cf0c3
into
PowerShell:master
|
Great work! |
|
Nice Work! |
…from hashtable splatting (PowerShell#13162) Allow explicitly specified named parameter to supersede the same one from hashtable splatting. The work is done in parameter binder, so that parameters can be resolved to cover a parameter's official name, alias name, and unambiguous partial prefix name. The changes covers covers Hashtable splatting in 3 scenarios: - Cmdlet or advanced script invocation; - Simple function invocation; - ScriptBlock.GetPowerShell(...), where the script block contains command invocation only and uses Hashtable splatting. Some code refactoring is done to ParameterBinderController to avoid redundant code being duplicated in CmdletParameterBinderController and ScriptParameterBinderController.
|
@daxian-dbw Should this be assigned to a milestone? |
|
Handy links: |
PR Summary
Fix #13114.
Allow explicitly specified named parameter to supersede the same one from hashtable splatting.
The work is done in parameter binder, so that parameters can be resolved to cover a parameter's official name, alias name, and unambiguous partial prefix name.
The changes covers covers Hashtable splatting in 3 scenarios:
ScriptBlock.GetPowerShell(...), where the script block contains command invocation only and uses Hashtable splatting.Some code refactoring is done to
ParameterBinderControllerto avoid redundant code being duplicated inCmdletParameterBinderControllerandScriptParameterBinderController.Breaking Change
This change introduces a minor breaking change to how Hashtable splatting works with simple functions when it contains key/value pairs that not a named parameters of the function. (no breaking change to cmdlet/advanced function)
With this change, the named parameters from Hashtable splatting will be moved to the end of the parameter/argument list, so as to late bind them after all explicitly specified named parameters are bound. Parameter binding for simple functions won't throw error when a specified named parameter cannot be found, but stuff the unknown named parameters to the
$argsof the simple function. Since the named parameters from Hashtable splatting are moved to the end of the argument list, the order it appears inargswill be different, also, other explicitly specified parameters for a simple function gets bound earlier.For example:
Current Behavior
New Behavior
I think the breaking change is in Bucket 3
Unlikely Grey Areabecause:$argsto be used in a real scenario like this.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.