Skip to content
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

Switch to Register-ArgumentCompleter for tab expansion #609

Closed

Conversation

cspotcode
Copy link

I saw talk of switching to the more modern Register-ArgumentCompleter for tab completion. This is the most minimal implementation of such a switch. Completion logic is the same. We register ourselves as a -Native completer for git, gitk, and tgit. When invoked, we convert the command AST into a string, trim it to match what the old completion logic is expecting, and pass it along.

}
}
@('git', 'tgit', 'gitk') | ForEach-Object {
Register-ArgumentCompleter -CommandName $_ -Native -ScriptBlock {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work on Windows PowerShell? My experience with the built-in Register-ArgumentCompleter in Windows PowerShell 5.1 is that it won't auto-complete native parameters that start with - or --. This would be a serious problem for Git.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, you're correct, it does not work. :( I'll post context.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it sounds like we want to use Register-ArgumentCompleter on PS 6+, and fall back on the previous behavior for PS 5.1?

@cspotcode
Copy link
Author

I didn't realize the built-in Register-ArgumentCompleter on Windows PowerShell has a bug where it doesn't tab-complete arguments that begin with a hyphen.

MicrosoftDocs/PowerShell-Docs#1979
PowerShell/PowerShell#2912

So I guess this is only useful if you want posh-git to get the benefits of built-in Register-ArgumentCompleter on new-enough versions of Powershell and fallback to the old logic on e.g. Windows PowerShell. The benefits of built-in Register-ArgumentCompleter are that you can tab complete e.g. after the "b" in this example:

echo $branches | ? { git b<# <-- cursor here #> } | Out-Null

@rkeithhill
Copy link
Collaborator

I've been looking at this. I think the following could work:

if ($PSVersionTable.PSVersion.Major -ge 6) {
    Register-ArgumentCompleter -CommandName git,tgit,gitk -Native -ScriptBlock {
        param($wordToComplete, $commandAst, $cursorPosition)

        Expand-GitCommand $commandAst.Extent.Text
    }
}
else {
    $PowerTab_RegisterTabExpansion = if (Get-Module -Name powertab) { Get-Command Register-TabExpansion -Module powertab -ErrorAction SilentlyContinue }
    if ($PowerTab_RegisterTabExpansion) {
        & $PowerTab_RegisterTabExpansion "git.exe" -Type Command {
            param($Context, [ref]$TabExpansionHasOutput, [ref]$QuoteSpaces)  # 1:

            $line = $Context.Line
            $lastBlock = [regex]::Split($line, '[|;]')[-1].TrimStart()
            $TabExpansionHasOutput.Value = $true
            Expand-GitCommand $lastBlock
        }
        return
    }

    if (Test-Path Function:\TabExpansion) {
        Rename-Item Function:\TabExpansion TabExpansionBackup
    }

    function TabExpansion($line, $lastWord) {
        $lastBlock = [regex]::Split($line, '[|;]')[-1].TrimStart()

        switch -regex ($lastBlock) {
            # Execute git tab completion for all git-related commands
            "^$(Get-AliasPattern git) (.*)" { Expand-GitCommand $lastBlock }
            "^$(Get-AliasPattern tgit) (.*)" { Expand-GitCommand $lastBlock }
            "^$(Get-AliasPattern gitk) (.*)" { Expand-GitCommand $lastBlock }

            # Fall back on existing tab expansion
            default {
                if (Test-Path Function:\TabExpansionBackup) {
                    TabExpansionBackup $line $lastWord
                }
            }
        }
    }
}

If you'd like to make this change, then I will accept and merge it. Also, in looking at the completer impl, I wasn't sure why we needed the padding/substring calls. Register-ArgumentCompleter seems to do a pretty good job of supplying just the command up to the param to complete in $commandAst.Extent.Text.

If you don't have time for this, I can submit a PR to do the above but I'll make sure you get credit for this in the changelog.

@dahlbyk I think it would be good to do one more beta drop with this change.

@rkeithhill
Copy link
Collaborator

Close in favor of #711 @cspotcode I'll make sure you get the credit for this in the changelog. Thanks for investigating this and doing the work!!

@rkeithhill rkeithhill closed this Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants