GeezerVison

Hi Dave Wyatt (@dlwyatt), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

We have a tool to update the workflow activity files that we run periodically so that you don't need to worry about it every time you add a new parameter to a cmdlet.

Travis CI build blew up, not sure why.

We've discussed this PR some internally, here is a summary - just points for discussion at this point.

• The breaking change could avoided by added a new parameter like AdditionalChildPath with ValueFromRemainingArguments = true.
• ChildPath accepting ValueFromRemainingArguments can be confusing, e.g. Join-Path root d1,d2 would work like Join-Path root "d1 d2"

The breaking change could avoided by added a new parameter like AdditionalChildPath with ValueFromRemainingArguments = true.

True. The output from Get-Command / Get-Help would be a little bit more cluttered, but that's no big deal if there's concern about changing existing parameter types. (There might also be weird situations where someone leaves ChildPath empty or null but passes values to AdditionalChildPaths. What's the proper behavior there? root//additional/child/paths or root/additional/child/paths?)

ChildPath accepting ValueFromRemainingArguments can be confusing, e.g. Join-Path root d1,d2 would work like Join-Path root "d1 d2"

Not sure I understand this one. Did you mean Join-Path "d1" "d2", or was the single string literal intended? How is this different from any other parameter with ValueFromRemainingArguments enabled?

Personally, I like having ValueFromRemainingArguments. I don't know if I'm in the minority on this, but Join-Path is one of the cmdlets that I always use with arguments passed by position rather than by name. It would be more confusing to me to have to remember where to start including commas, instead of having a simpler call. Assuming we leave -ChildPath alone and add a third parameter, the calls look like:

Join-Path one two three,four,five

# versus

Join-Path one two three four five

I think the second syntax looks much nicer (and less error prone), but even if I had to go with the first option, it would still be useful.

No, I meant what I wrote - try it:

#57 PS> & {
>>     param(
>>         [string[]]
>>         [Parameter(Position=0)]
>>         $Path,
>>
>>         [string[]]
>>         [Parameter(Position=1, ValueFromRemainingArguments)]
>>         $Extra)
>>     $Extra.Count;
>>     $Extra | % { "arg: $_"}
>> } root aa,bb
1
arg: aa bb

I do think your example is fine w/ no commas, but folks do learn to use commas, and sometimes it's surprising. This isn't specific to your usage, it applies to ValueFromRemainingArguments in general.

That doesn't make any sense. Seems like a bug in the parameter binder to me. It shouldn't matter whether you pass remaining arguments or an explicit array, both should wind up with identical values in the command.

On Aug 22, 2016, at 7:20 PM, Jason Shirk [email protected] wrote:

No, I meant what I wrote - try it:

#57 PS> & {

param(
    [string[]]
    [Parameter(Position=0)]
    $Path,

    [string[]]
    [Parameter(Position=1, ValueFromRemainingArguments)]
    $Extra)
$Extra.Count;
$Extra | % { "arg: $_"}

} root aa,bb
1
arg: aa bb
I do think your example is fine w/ no commas, but folks do learn to use commas, and sometimes it's surprising. This isn't specific to your usage, it applies to ValueFromRemainingArguments in general.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Interestingly, this problem doesn't affect cmdlets, just PowerShell functions:

 & {
     param(
         [string[]]
         [Parameter(Position=0)]
         $Path,

         [string[]]
         [Parameter(Position=1, ValueFromRemainingArguments)]
         $Extra)
     $Extra.Count;
     for ($i = 0; $i -lt $Extra.Count; $i++)
     {
        "${i}: $($Extra[$i])"
     }
 } root aa,bb

<#
1
0: aa bb
#>

Add-Type -OutputAssembly $env:temp\testBinding.dll -TypeDefinition @'
    using System;
    using System.Management.Automation;

    [Cmdlet("Test", "Binding")]
    public class TestBindingCommand : PSCmdlet
    {
        [Parameter(Position = 0)]
        public string Root { get; set; }

        [Parameter(Position = 1, ValueFromRemainingArguments = true)]
        public string[] Extra { get; set; }

        protected override void ProcessRecord()
        {
            WriteObject(Extra.Length);
            for (int i = 0; i < Extra.Length; i++)
            {
                WriteObject(String.Format("{0}: {1}", i, Extra[i]));
            }
        }
    }
'@

Import-Module $env:temp\testBinding.dll

Test-Binding root aa,bb

<#
2
0: aa
1: bb
#>

Logged issue #2035 for the different binding behavior between cmdlets and functions.

Binding bug should be fixed in #2038 .

Now that the binder thing is sorted out (even if the other PR isn't accepted due to being classified as a breaking change, it doesn't affect the Join-Path cmdlet anyway), looking back at this one. Would you prefer to change ChildPath back to the way it was and introduce a new parameter instead?

If so, what's your preference for behavior if someone passes arguments to the new parameter, but ChildPath is null or empty?

• Throw an error.
• Build the path with an empty element. (`/root//additional/paths')
• Build the path, ignoring the fact that ChildPath is empty. (/root/additional/paths)

Come to think of it, the same question applies if someone passes empty strings at any point in the new parameter. Ignore them, or have some doubled separators in the output? /root/one//three versus /root/one/three.

👍 I really like this Dave Wyatt (@dlwyatt)! This style is similar to ruby's File.join that takes N strings and just joins them all. It always felt more concise and natural than powershell's current implementation.

Regarding $null entries - it seems like it should be an a terminating error. Note that Join-Path supports piped arguments:

PS> 'a','b' | Join-Path -ChildPath c
a\c
b\c

And $null results in a non-terminating error. That said, if there are any $null values in -ChildPath, nothing from the pipeline would work, so a single terminating error seems better than multiple non-terminating errors.

'a','b' | Join-Path -ChildPath $null works fine today, you get this:

a\
b\

For whatever reason, ChildPath is marked as ValueFromPipelineByPropertyName, so if we were going to produce errors about it, nonterminating is probably the correct option. Also, ChildPath has [AllowNull()] but Path does not. No idea why they are different, but I left that alone. Only difference now is that you can have multiple ChildPath strings, and there are a few options for how to handle null / empty values in the middle of that array rather than at the end.

I'm leaning toward ignoring empty values unless they're at the end of the list, in which case we'll go ahead and tack on a trailing separator to keep behavior consistent with today.

Well, I can't say I like it, but it's at least consistent with string.Join:

PS> [string]::Join("\", 'a', [NullString]::Value, 'c')
a\\c

True, but the benefit of Join-Path is that you don't have dumb logic like that. It looks to see if either of the strings have leading / trailing separators already so it doesn't double them up. :)

Join-Path '\a\b\' '\c\d'
\a\b\c\d

In fact, that's how SessionState.Path.Combine is already working, so I'd have to go out of my way to wind up with doubled separators in the string. Seems weird to add extra code to produce goofy output, so problem solved?

$path = ''
$array = 'a', [nullstring]::Value, 'c'
foreach ($p in $array) { $path = $ExecutionContext.SessionState.Path.Combine($path, $p) }
$path

a\c

As discussed, moved new code to a new parameter. Didn't bother putting a default value back on ChildPath since it's mandatory anyway.

Closed via 22aac12 - I deleted a blank line between the comments and a property.