lzybkr / TabExpansionPlusPlus

A V3 PowerShell module to improve tab expansion and Intellisense
BSD 2-Clause "Simplified" License
196 stars 33 forks source link

Adding Test-ArgumentCompleter function #24

Closed powercode closed 10 years ago

powercode commented 10 years ago

Adding a function to make it easier to test the argument completers

powercode commented 10 years ago

The original test does not work well in a pipeline with multiple elements. I changed it because if the piped object does not contain a tooltip, all completionresults gets the tooltip of the first object in the pipeline. And the effect of this change is to set the tooltip equal to the CompletionText if non is specified.

powercode commented 10 years ago

I also have an Idea about adding a parameter WordToComplete to New-CompletionResult (defaulting to empty) so that a new CompletionResult is created only if CompletionText starts with WordToComplete.

What do you think?

I have written that functionality in all my completers, so it seems like something that should be in the module instead.

lzybkr commented 10 years ago

Can you give me an example regarding $tooltip? I must be misunderstanding because parameters aren't supposed to persist. E.g.:

filter foo
{
    param([parameter(ValueFromPipelineByPropertyName)]$abc,
          [parameter(ValueFromPipeline)]$def)
    "abc: $abc"
}
[pscustomobject]@{abc=1},"def" | foo

prints

abc: 1
abc:
lzybkr commented 10 years ago

Regarding a $wordToComplete parameter to New-CompletionResult, I think I understand what why you want it, but it feels like the wrong approach - New-* feels like it should always create something.

powercode commented 10 years ago

I must say I am a bit surprised by the behavior, but with the following, I get the same tooltip all over. Seems that we have the ToolTip variable in local scope, and when we do not rebind it in the pipeline, it keeps it's local value. (Look at the tooltip in the output) I think it happens when we assign $ToolTip a new value, PowerShell create a local scope variable (Copy on Write). This is what messes with us.

function New-CompletionResult
{
    param([Parameter(Position=0, ValueFromPipelineByPropertyName, Mandatory, ValueFromPipeline)]
          [ValidateNotNullOrEmpty()]
          [string]
          $CompletionText,

          [Parameter(Position=1, ValueFromPipelineByPropertyName)]
          [string]
          $ToolTip)

    process 
    {            
        if ($ToolTip -eq '')
        {
            $ToolTip = $CompletionText
        }        
        return New-Object System.Management.Automation.CompletionResult `
            ($CompletionText,$CompletionText,'ParameterValue',$ToolTip.Trim())
    }

}
'a','b','c' | New-CompletionResult
CompletionText               ListItemText                                  ResultType ToolTip                    
--------------               ------------                                  ---------- -------                    
a                            a                                         ParameterValue a                          
b                            b                                         ParameterValue a                          
c                            c                                         ParameterValue a           
powercode commented 10 years ago

I agree that it feels hacky to have the filtering in New-CompletionResult. But it is needed, and the same could be said to be true with -WhatIf.

It also changes the behavior by not creating the resource.

lzybkr commented 10 years ago

Hmm, you found a bug in PowerShell - a similar script in V2 behaves as expected. I'll take care of reporting (and likely fixing) the bug.

Now that I understand the issue, I prefer a different change. I would introduce a new variable instead of assigning to the parameter, something like:

$toolTipToUse = if ($ToolTip -eq '') { $CompletionText } else { $ToolTip }

then use $toolTipToUse instead of $ToolTip. This should satisfy both of us.

lzybkr commented 10 years ago

I was suggesting the New is the wrong verb for the behavior you want to add to New-CompletionResult. I would consider another command though - it would definitely be nice to clean up the common pattern of

something | Where-Object { $_ -like "$wordToComplete*" } | ForEach-Object { New-CompletionResult ... }

Maybe the verb is Get-CompletionResult:

something | Get-CompletionResult $wordToComplete
powercode commented 10 years ago

Yes, why not use Get?

That keeps New clean.

lzybkr commented 10 years ago

I guess duplicate the code. Proxies are ugly and most useful when you don't control the code you are proxying.


From: Staffan Gustafssonmailto:notifications@github.com Sent: ý11/ý25/ý2013 12:25 AM To: lzybkr/TabExpansionPlusPlusmailto:TabExpansionPlusPlus@noreply.github.com Cc: Jasonmailto:jason@truewheels.net Subject: Re: [TabExpansionPlusPlus] Adding Test-ArgumentCompleter function (#24)

Yes, why not use Get?

That keeps New clean. Duplicate the code or write a proxy function?

— Reply to this email directly or view it on GitHubhttps://github.com/lzybkr/TabExpansionPlusPlus/pull/24#issuecomment-29184136.

powercode commented 10 years ago

I realized I'm an idiot :)

    if ($CompletionText.StartsWith($WordToComplete)) 
    {
        New-CompletionResult ...
    }

But when thinking about it, Get doesn't really seem right either. "Specifies an action that retrieves a resource. " It is some sort of new.

Leaning against that New with a -Filter is better than Get. Or New-CompletionResultIfMatching, but it is kinda long. New-CompletionResultIf ?

lzybkr commented 10 years ago

StartsWith is insufficient - $wordToComplete could use wildcards.

Get isn't perfect either, but I'll accept it. It's enough like other cmdlets, e.g

Get-CimInstance -Filter ... 

CIM instances don't necessarily exist before you call the cmdlet, so it's not that different. ConvertTo crossed my mind, but that feels weird. Select is the only other verb that might make sense, but it's not used often so I'm not too sure about it.