choco icon indicating copy to clipboard operation
choco copied to clipboard

(#3364) Fix broken tab completion (expansion) in PowerShell v7.4+

Open mklement0 opened this issue 2 years ago • 3 comments

Description Of Changes

Switches to using Register-ArgumentCompleter for tab-completion (expansion) when running in PowerShell v7.4+

Motivation and Context

As discussed in #3364:

  • Chocolatey's tab completion broke in v7.4.0, due to an intentional breaking change in PowerShell (removal of legacy code, as a side effect of which PowerShell no longer calls custom TabExpansion functions, if defined).

  • The recommended approach (since v5.0) is to use Register-ArgumentCompleter

Testing

  • Interactively (tab completion).

  • Programmatically, by calling TabExpansion2; e.g:

(TabExpansion2 ($c = 'choco c') -cursorColumn $c.Length).CompletionMatches.CompletionText | 
  Should -Be cache, config
(TabExpansion2 ($c = 'choco l later') -cursorColumn 7).CompletionMatches.CompletionText |
  Should -Be list

Operating Systems Testing

Windows 11 22H2

Change Types Made

  • [x] Bug fix (non-breaking change).
  • [ ] Feature / Enhancement (non-breaking change).
  • [ ] Breaking change (fix or feature that could cause existing functionality to change).
  • [ ] Documentation changes.
  • [x] PowerShell code changes.

Change Checklist

  • [ ] Requires a change to the documentation.
  • [ ] Documentation has been updated.
  • [ ] Tests to cover my changes, have been added.
  • [ ] All new and existing tests passed?
  • [ ] PowerShell code changes: PowerShell v2 compatibility checked?

I don't have access to PowerShell v2, but I think the changes are compatible.

Related Issue

Fixes #3364

mklement0 avatar Jan 19 '24 18:01 mklement0

How do you think about using Register-ArgumentCompleter on PowerShell >= 5.0 instead of PowerShell >= 7.4? That will help https://github.com/chocolatey/choco/issues/2255 for users on a little bit older PowerShell.

yan12125 avatar Jan 19 '24 18:01 yan12125

I tried to be conservative, but, yes, it should be possible to use Register-ArgumentCompleter in v5+

However, support for choco.exe is really a separate issue (I just happened to add it to the v7.4+ code - though I just reverted it, because it was incomplete and introduces an asymmetry).

The only thing needed to enable it for the v7.3- code is the following:

Change the $aliases=... line in:

function Get-AliasPattern($exe) {
    $aliases = @($exe) + @(Get-Alias | Where-Object { $_.Definition -eq $exe } | Select-Object -Exp Name)

    "($($aliases -join '|'))"
}

to:

    $aliases = @($exe, "$exe\.exe") + @(Get-Alias | Where-Object { $exe, "$exe.exe" -contains $_.Definition } | Select-Object -Exp Name)

If the maintainers are fine with this, I'm happy to update the PR to add choco.exe support in all versions.

mklement0 avatar Jan 19 '24 18:01 mklement0

Oops, I misunderstood the issue about choco.exe. Many thanks for the kind explanation!

yan12125 avatar Jan 20 '24 04:01 yan12125

There are merge conflicts in src/chocolatey.resources/helpers/chocolateyProfile.psm1 after commit https://github.com/chocolatey/choco/commit/186ffc840f91944f5939080cbc78140f71ec4506. Mind to do a rebase?

This PR lgtm. Why it's still not merged?

It seems pull requests towards Chocolatey repositories are often kept open for a long time. My other pull request has been there for almost one year.

yan12125 avatar May 26 '24 17:05 yan12125

@yan12125, it looks like the only reason for the conflict is that a signature was added, which I cannot recreate myself.

mklement0 avatar May 26 '24 17:05 mklement0

which I cannot recreate myself.

Do you mean that you cannot create new signatures? Apparently new signatures will be created by maintainers after scripts are updated. For example, after scripts are changed in dc409a3de0c03a29d95fec73a555a5d4fedc4ac3, signatures are updated in d8821c8c1b3c00a008aaaf44f879b8503881d4c6. As a result, I'd assume it's enough to resolve conflicts in scripts without touching signatures.

yan12125 avatar May 26 '24 17:05 yan12125

@yan12125, yes, I meant the signature. I took your advice and resolved the conflict while keeping the - by definition now invalid - signature.

mklement0 avatar May 26 '24 18:05 mklement0

Thank you! Hopefully some dev will have a look at this soon.

yan12125 avatar May 27 '24 05:05 yan12125

@mklement0 thank you for taking the time for creating this PR, and for getting it updated.

Before we will be able to look at this, and get it merged in, the PR will need to conform to the guidelines that are stipulated in the CONTRIBUTING document for this repository. Can I get you to look through that document, specifically the "prepare commits` section.

gep13 avatar May 27 '24 06:05 gep13

@gep13, I fully understand that, as a high-profile project, you need to enforce standards on community PRs.

However, personally, as an infrequent Chocolatey user, I'm not willing to invest the time to learn how to conform to those standards.

I hope someone else is, though, and they're free to base their future PR on the code in this one.

mklement0 avatar May 28 '24 21:05 mklement0

I hope someone else is, though, and they're free to base their future PR on the code in this one.

Thanks, I created a new pull request from your codes https://github.com/chocolatey/choco/pull/3459

yan12125 avatar Jun 01 '24 13:06 yan12125

Thank you, @yan12125.

mklement0 avatar Jun 01 '24 16:06 mklement0