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

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

yan12125
Copy link

@yan12125 yan12125 commented Jun 1, 2024

(A continuation of #3387. Most descriptions below are copied from there)

Description Of Changes

Fix broken tab completion (expansion) in PowerShell v7.4+ by using the new Register-ArgumentCompleter API

TabExpansion is not used since PowerShell 7.4, so the export of legacy TabExpansion function is made conditional on the PowerShell version.

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

Operating Systems Testing

Windows 10 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

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.

I'm already using a few weeks.

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

@yan12125
Copy link
Author

yan12125 commented Jun 1, 2024

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

Hey, although this pull request is identical to #3387, it's open only for one hour :D

@silverqx
Copy link

silverqx commented Jun 1, 2024

That is the reason why I copy-pasted the review as I compared the diff and not even one char changed 😅

@yan12125
Copy link
Author

yan12125 commented Jun 6, 2024

Rebased to fix conflicts with 576527b.

@gep13 Mind to have a look?

@silverqx
Copy link

choco v2.3.0 is out and auto-completion still doesn't work 🤔

But we can be happy that all commands are now returning correct exit codes in edge cases 😅 Release notes

Why don't you merge this PR? Is there any problem with it? @gep13

It looks like even a corrupted feature is better than PR. 🙃

The good news is that the completion of package names started working after upgrading to choco v2.3 and applying this PR. 👌(I think it didn't work before?)

@silverqx
Copy link

Now I also looked over the Blog post about a new release and any of these new features had higher priority than this issue.

@gep13
Copy link
Member

gep13 commented Jun 11, 2024

@silverqx at no point was there a commitment that this issue was going to get resolved in the 2.3.0. If there was something that gave you that impression, I would like to no what, so that this can't happen again.

The issue associated with this PR has been added to the 2.4.0 milestone.

@silverqx said...
It looks like even a corrupted feature is better than PR.

Can I please ask what you mean by this, as it isn't immediately clear what you are referring to.

@silverqx
Copy link

The issue associated with this PR has been added to the 2.4.0 milestone.

Thx, I'm happy with this 👍, I believe a lot of people are struggling because of this regression, all other projects were fixed months ago.

@gep13
Copy link
Member

gep13 commented Jun 11, 2024

@silverqx said...
I believe a lot of people are struggling because of this regression, all other projects were fixed months ago.

Again, I am not clear on what you are referring to here...

Can you please explain further both this comment, and your previous comment.

vexx32
vexx32 previously requested changes Oct 28, 2024
Copy link
Member

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

I have a couple questions on this one but the base functionality here seems very sound. Great work! It seems to work just fine in PowerShell 7.4 and also Windows PowerShell 5.1

We are looking to pull this in for 2.4.0, so if you are unavailable for the time being we may make the suggested minor changes and look to pull it in shortly. Either way, thank you very much for the contribution!

src/chocolatey.resources/helpers/chocolateyProfile.psm1 Outdated Show resolved Hide resolved
@vexx32
Copy link
Member

vexx32 commented Nov 1, 2024

@yan12125 I've amended the PR according to the suggestions (mostly... I don't know if it's worth having a function to version check a simple $PSVersionTable.PSVersion.Major -lt 5) and rebased it on develop.

I will hand this over to @corbob for final review now. Thank you for your contribution!

@vexx32 vexx32 requested a review from corbob November 1, 2024 19:50
@corbob corbob dismissed vexx32’s stale review November 1, 2024 22:50

These have been addressed.

Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

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

This looks good and is working as expected for me.

I will hold off on merging it for now as merging this will break scheduled builds over the weekend. Will get it merged in on Monday.

@corbob
Copy link
Member

corbob commented Nov 1, 2024

Thank you @yan12125 for submitting this PR, and to @mklement0 for the initial PR this one is based off of.

@yan12125
Copy link
Author

yan12125 commented Nov 3, 2024

Thank you all for updating and reviewing this!

mklement0 and others added 2 commits November 4, 2024 10:49
… v7.4+

By using the new Register-ArgumentCompleter API

TabExpansion is not used since PowerShell 7.4, so the export of legacy
`TabExpansion` function is made conditional on the PowerShell version.
The newer method is available from v5 and up, so we have no need to wait
for v7.4+ to change over. This will also let us remove the fallback
earlier, once we no longer support PS <5.0.
@corbob corbob merged commit 4947075 into chocolatey:develop Nov 4, 2024
5 checks passed
@corbob corbob mentioned this pull request Nov 4, 2024
10 tasks
@yan12125 yan12125 deleted the fix-issue3364 branch November 4, 2024 23:37
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
6 participants