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

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

Closed
wants to merge 4 commits into from

Conversation

mklement0
Copy link

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

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • 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

Make export of legacy `TabExpansion` function conditional on the PowerShell version.
@yan12125
Copy link

How do you think about using Register-ArgumentCompleter on PowerShell >= 5.0 instead of PowerShell >= 7.4? That will help #2255 for users on a little bit older PowerShell.

Reverted the addition of "choco.exe" to the list of aliases, for symmetry with the legacy code.
@mklement0
Copy link
Author

mklement0 commented Jan 19, 2024

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.

@yan12125
Copy link

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

Copy link

@silverqx silverqx left a comment

Choose a reason for hiding this comment

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

I tried this PR and it works well, everything looks good short, long parameters, completion of commands and also completion of package names.

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

@yan12125
Copy link

There are merge conflicts in src/chocolatey.resources/helpers/chocolateyProfile.psm1 after commit 186ffc8. 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.

@mklement0
Copy link
Author

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

@yan12125
Copy link

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 dc409a3, signatures are updated in d8821c8. As a result, I'd assume it's enough to resolve conflicts in scripts without touching signatures.

@mklement0
Copy link
Author

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

@yan12125
Copy link

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

@gep13
Copy link
Member

gep13 commented May 27, 2024

@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.

@mklement0
Copy link
Author

@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.

@yan12125
Copy link

yan12125 commented Jun 1, 2024

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 #3459

@mklement0
Copy link
Author

Thank you, @yan12125.

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.

Tab completion does not work since PowerShell 7.4
4 participants