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

Fish does not accept dash or colon in vars #1122

Merged
merged 1 commit into from Jul 10, 2020

Conversation

marckhouzam
Copy link
Collaborator

Fixes #1121.

This is for programs that may contain a : or - in their name.

Those characters are not accepted in variables for fish, so it was breaking fish completion.
This PR replaces - and : with a _ in the fish completions script.

@sayboras
Copy link
Contributor

@marckhouzam thanks for your fix, I just tested with your branch (for golangci-lint), and it's working fine 💯. Hope this one will land in soon 💯

@jharshman jharshman added area/shell-completion All shell completions kind/bug A bug in cobra; unintended behavior labels May 29, 2020
@blaggacao
Copy link

Another bitten one at git-town ... ⌛

Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

I've tested this in my fish environment and with some test commands. Thanks for adding tests for this too!

lgtm!

@marckhouzam
Copy link
Collaborator Author

This has been rebased to master after the merging of #1070.

Maybe @sayboras and/or @blaggacao can confirm it still works for their project?

I did test on my side by adding a - and a : to my helm binary name and it failed before this PR but worked with this PR.

Fixes spf13#1121.
This is for programs that may contain a : or - in their name.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
@sayboras
Copy link
Contributor

@marckhouzam seems like it didn't work anymore, I haven't enabled debug to see more details though. Will do once I have some free time.

FYI. I am using the below replace directive statement.

replace github.com/spf13/cobra => github.com/VilledeMontreal/cobra v0.0.6-0.20200629203744-5782fedb5fb1

@marckhouzam
Copy link
Collaborator Author

replace github.com/spf13/cobra => github.com/VilledeMontreal/cobra v0.0.6-0.20200629203744-5782fedb5fb1

@sayboras Works for me 😄

I assume you are working with golangci-lint based on your previous comment. I cloned it and used your changes from golangci/golangci-lint#1028 to enable fish completion, and I added the replace command you mentioned above. See below for the patch of changes.

Then I ran:

$ make clean;make build
$ fish
$ ./golangci-lint completion fish |source
$ ./golangci-lint completion <TAB>
bash  (Output bash completion script)  fish  (Output fish completion script)  zsh  (Output zsh completion script)

If I remove the replace instruction from go.mod then triggering completion does fail with a messy fish script error.

This is the patch I applied on top of master to test things. You can save it to a file, say fish.patch and then run git am fish.patch` to apply it:

From 98d969d37dc34066e50d954e40190a3fec7c742b Mon Sep 17 00:00:00 2001
From: Marc Khouzam <marc.khouzam@montreal.ca>
Date: Mon, 29 Jun 2020 22:02:31 -0400
Subject: [PATCH] TMP

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
---
 go.mod                     |  2 ++
 go.sum                     |  2 ++
 pkg/commands/completion.go | 15 +++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/go.mod b/go.mod
index 208b680..6be5cf0 100644
--- a/go.mod
+++ b/go.mod
@@ -62,3 +62,5 @@ require (
 	mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b // indirect
 	mvdan.cc/unparam v0.0.0-20190720180237-d51796306d8f
 )
+
+replace github.com/spf13/cobra => github.com/VilledeMontreal/cobra v0.0.6-0.20200629203744-5782fedb5fb1
diff --git a/go.sum b/go.sum
index 5b84616..6e07489 100644
--- a/go.sum
+++ b/go.sum
@@ -23,6 +23,8 @@ github.com/OpenPeeDeeP/depguard v1.0.1 h1:VlW4R6jmBIv3/u1JNlawEvJMM4J+dPORPaZasQ
 github.com/OpenPeeDeeP/depguard v1.0.1/go.mod h1:xsIw86fROiiwelg+jB2uM9PiKihMMmUx/1V+TNhjQvM=
 github.com/StackExchange/wmi v0.0.0-20180116203802-5d049714c4a6 h1:fLjPD/aNc3UIOA6tDi6QXUemppXK3P9BI7mr2hd6gx8=
 github.com/StackExchange/wmi v0.0.0-20180116203802-5d049714c4a6/go.mod h1:3eOhrUMpNV+6aFIbp5/iudMxNCF27Vw2OZgy4xEx0Fg=
+github.com/VilledeMontreal/cobra v0.0.6-0.20200629203744-5782fedb5fb1 h1:b4N6UrpLCX/ClvjZnqRuTxLfLAF0+5tqWJLT2D/Zx2g=
+github.com/VilledeMontreal/cobra v0.0.6-0.20200629203744-5782fedb5fb1/go.mod h1:/6GTrnGXV9HjY+aR4k0oJ5tcvakLuG6EuKReYlHNrgE=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
 github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e/go.mod h1:3U/XgcO3hCbHZ8TKRvWD2dDTCfh9M9ya+I9JpbB7O8o=
diff --git a/pkg/commands/completion.go b/pkg/commands/completion.go
index 7d919d1..0edade7 100644
--- a/pkg/commands/completion.go
+++ b/pkg/commands/completion.go
@@ -28,6 +28,13 @@ func (e *Executor) initCompletion() {
 		RunE:  e.executeZshCompletion,
 	}
 	completionCmd.AddCommand(zshCmd)
+
+	fishCmd := &cobra.Command{
+		Use:   "fish",
+		Short: "Output fish completion script",
+		RunE:  e.executeFishCompletion,
+	}
+	completionCmd.AddCommand(fishCmd)
 }

 func (e *Executor) executeBashCompletion(cmd *cobra.Command, args []string) error {
@@ -51,3 +58,11 @@ func (e *Executor) executeZshCompletion(cmd *cobra.Command, args []string) error

 	return nil
 }
+
+func (e *Executor) executeFishCompletion(cmd *cobra.Command, args []string) error {
+	err := cmd.Root().GenFishCompletion(os.Stdout, true)
+	if err != nil {
+		return errors.Wrap(err, "unable to generate fish completions: %v")
+	}
+	return nil
+}
--
2.23.0

@sayboras
Copy link
Contributor

@marckhouzam oops, I had the same changes, but used the wrong binary to test. It's working fine for me :), sorry for any inconvenience caused :(

image

@marckhouzam
Copy link
Collaborator Author

@sayboras no problem. Thanks for confirming it works.

Copy link
Contributor

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM :)

@blaggacao
Copy link

I'd love to give my review, but I'm currently knee deep in another context and can't switch that easily. I hope, my opinion is not needed.

@jharshman jharshman merged commit 675ae5f into spf13:master Jul 10, 2020
@marckhouzam marckhouzam deleted the fix/fishDash branch July 11, 2020 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/shell-completion All shell completions kind/bug A bug in cobra; unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

functions in fish shell completion auto-generated file should not contain "-" character
5 participants