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

[RFC] Redirect bash completion v1 to v2 when possible #1867

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marckhouzam
Copy link
Collaborator

We are no longer actively maintaining bash completion v1 in favor of its more rich v2 version. Previously, using bash completion v2 required projects to be aware of its existence and to explicitly call GenBashCompletionV2().

With this commit, any projects calling GenBashCompletion() will automatically be redirected to using the v2 version.
One exception is if the project uses the legacy custom completion logic which is not supported in v2. We can detect that by looking for the use of the field BashCompletionFunction on the root command.

If we ignore the tests and doc, this is a 2 line change in bash_completions.go

Note that descriptions are kept off when calling GenBashCompletion(). This means that to enable completion descriptions for bash, a project must still explicitly call GenBashCompletionV2(). I chose this approach because I was hesitant to enable descriptions without getting projects to make the decision themselves.

cc @jpmcb @scop @Luap99

@marckhouzam marckhouzam added kind/feature A feature request for cobra; new or enhanced behavior area/shell-completion All shell completions labels Nov 24, 2022
Copy link
Contributor

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

I am ok with the change but I wonder what the motivation is? Assuming that you will not remove the old script until a major version bump I don't see the benefit.

bash_completions.go Show resolved Hide resolved
@marckhouzam
Copy link
Collaborator Author

I am ok with the change but I wonder what the motivation is? Assuming that you will not remove the old script until a major version bump I don't see the benefit.

I should have explained in the PR description, sorry.
Let me start by saying that I agree this is not a clearcut decision, and I welcome any input for or against it.

Basically I'm trying to improve the quality of bash completion for projects using Cobra. I don't think it is easy for projects to know that there is such a thing as a V2 version and therefore I feel projects are missing out on bug fixes and features.
I opted to propose this change after opening this bug vmware-tanzu/tanzu-framework#3963; the bug made me realize that bash completion can be affected by subtle bugs that may go unreported/unfixed although they have already been addressed by Cobra's bash completion V2 solution.

But maybe this is overreaching. I'd like people's honest opinion on this and if there is too much hesitation, we'll just close it.

We are no longer actively maintaining bash completion v1 in favor of its
more rich v2 version.  Previously, using bash completion v2 required
projects to be aware of its existence and to explicitly call
GenBashCompletionV2().

With this commit, any projects calling GenBashCompletion() will
automatically be redirected to using the v2 version.

One exception is if the project uses the legacy custom completion logic
which is not supported in v2.  We can detect that by looking for the
use of the field `BashCompletionFunction` on the root command.

Note that descriptions are kept off when calling GenBashCompletion().
This means that to enable completion descriptions for bash, a project
must still explicitly call GenBashCompletionV2().

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
@github-actions github-actions bot removed the area/shell-completion All shell completions label Nov 25, 2022
@marckhouzam marckhouzam changed the title Redirect bash completion v1 to v2 when possible [RFC] Redirect bash completion v1 to v2 when possible Nov 25, 2022
@Luap99
Copy link
Contributor

Luap99 commented Dec 2, 2022

I think it is a good idea overall. IMO there shouldn't be a reason to use the V1 version but then again I don't really know how and where is is used. One thing that we can do without changing behaviour is to add a // Deprecated: comment which will be detected by some linters, i.e. https://staticcheck.io/docs/checks/#SA1019, and make authors aware that they should switch.

@scop
Copy link
Contributor

scop commented Dec 4, 2022

Seems fine to me too. Deprecated: on the original functions seconded, more info at https://github.com/golang/go/wiki/Deprecated

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
    You can:
  • Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interested in reopening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A feature request for cobra; new or enhanced behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants