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

fix(config): move --install-tab-completion to its own command #703

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

amalthundiyil
Copy link
Contributor

@amalthundiyil amalthundiyil commented Aug 23, 2022

Description

The problem with the earlier approach is that the --install-tab-completion which should be an option is also trying to play the role of a command (also see koordinates/kart#648 (review)). This PR adds a new command instead of adding it as an option in a subcommand.

kart install tab-completion # to auto-detect shell and install tab completion

Related links:

Checklist:

  • Have you reviewed your own change?
  • Have you included test(s)?
  • Have you updated the changelog?

@rcoup
Copy link
Member

rcoup commented Aug 23, 2022

I feel like we might end up with other setup commands which are only useful once.

(I think it was my suggestion to move it under config, sorry)

Thoughts @craigds @olsen232 ?

@olsen232
Copy link
Collaborator

Hard to argue with the premise - it is a command, not an option, so if we make any distinction, that's where it should go.

It could be a subcommand if we don't want it at the top-level - if this is too much prominence for a one-off command - or if we expect to have more one-off commands like this.

It could maybe be kart config install-tab-completion (not sure if we can subcommand kart config, or if it has to be a special "group" command) or it could be kart install tab-completion

I think probably - perhaps more importantly than exactly what we call our commands - we need to one day customise the kart --help text so that it can give prominence to some commands ahead of others, maybe add some headings eg "common commands", "advanced", "setup commands"

@craigds
Copy link
Member

craigds commented Aug 24, 2022

LGTM :shipit:

@craigds craigds merged commit 2ca3b0a into koordinates:master Aug 24, 2022
@amalthundiyil amalthundiyil deleted the fix branch August 24, 2022 07:04
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.

4 participants