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

Pass context to completion #1265

Merged
merged 1 commit into from May 3, 2021
Merged

Pass context to completion #1265

merged 1 commit into from May 3, 2021

Conversation

@lukasmalkmus
Copy link
Contributor

@lukasmalkmus lukasmalkmus commented Oct 25, 2020

Closes #1260.
Closes #1263.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Oct 25, 2020

CLA assistant check
All committers have signed the CLA.

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Oct 26, 2020

Thanks @lukasmalkmus. Could you give a snippet of code that shows how a context is set so I can test? I've never used one but I assume it should be set on the root command?

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Oct 26, 2020

Yes, use rootCommand.ExecuteContext().

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Nov 26, 2020

@lukasmalkmus I've never used go contexts before. Could you provide a small test program to show how you would generate the context and then how you would use it?

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Nov 27, 2020

I'll extend this example from the docs:https://github.com/spf13/cobra/blob/master/shell_completions.md#dynamic-completion-of-nouns

So imagine you got the linked command and want to do dynamic completion of nouns:

cmd := &cobra.Command{
	Use:   "status RELEASE_NAME",
	Short: "Display the status of the named release",
	Long:  status_long,
	RunE: func(cmd *cobra.Command, args []string) {
		RunGet(args[0])
	},
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) != 0 {
			return nil, cobra.ShellCompDirectiveNoFileComp
		}
		return getReleasesFromCluster(toComplete), cobra.ShellCompDirectiveNoFileComp
	},
}

Now we might want to use a context in our application for various reasons (cancelation, passing metadata (gRPC), etc.):

cmd := &cobra.Command{
	Use:   "status RELEASE_NAME",
	Short: "Display the status of the named release",
	Long:  status_long,
	RunE: func(cmd *cobra.Command, args []string) {
		RunGet(args[0])
	},
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) != 0 {
			return nil, cobra.ShellCompDirectiveNoFileComp
		}
		return getReleasesFromCluster(cmd.Context(), toComplete), cobra.ShellCompDirectiveNoFileComp
	},
}

func getReleasesFromCluster(ctx context.Context, toComplete string) []string {
	req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://serve.com/api/releases", nil)
	if err != nil {
		return nil
	}

	resp, err := http.DefaultClient.Do(req)
	if err != nil {
		return nil
	}

	// ...

	 return releases
}

To actually inject the context, it must be passed using cmd.ExecuteContext(). However, this currently fails on master because the context isn't passed down into the ValidArgsFunction.

I also want to use the functionally of cmd.ExecuteC() but still need the context. That's why I added cmd.ExecuteContextC().

Feel free to reach out if you need more details.

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Nov 27, 2020

Just noticed I didn't add ExecuteContextC as requested in #1260 😅

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Nov 27, 2020

And added it. This now closes #1260 as well.

@jharshman jharshman added this to the Next milestone Dec 18, 2020
@jharshman
Copy link
Collaborator

@jharshman jharshman commented Dec 18, 2020

@lukasmalkmus can you add some tests?

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Dec 19, 2020

@jharshman Done! I updated the helper function executeCommandWithContext to use ExecuteContextC under the hood. The implementation of ExecuteContextC is ridiculously simple but this change makes sure it gets some usage in tests.

I also added TestValidArgsFuncCmdContext which makes sure a valid context is received in a ValidArgsFunc.

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Dec 20, 2020

Can someone unmark #1260 as being stale?

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Thanks for this. I added a couple of comments in-line.

command_test.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Dec 28, 2020

Will address soon, thanks for you in depth review. Should have spent some more time on that myself tbh.

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Jan 17, 2021

I was short on time but everything should work out now.

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Jan 17, 2021

Test fails are not related to my changes. I just need #1309 :)

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Thanks @lukasmalkmus.

I explained better (I hope) why I think it is important to test ExecuteContext().

The rest looks good.
Can you also rebase the PR to the latest master? #1309 has just been merged.

command_test.go Outdated Show resolved Hide resolved
@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Jan 17, 2021

Resolved all discussions and rebased on latest master.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

To looks good to me. Thanks @lukasmalkmus for addressing all comments.
One last thing is that usually I'm asked to squash all the commits into one before the PR is merged.

@jharshman I believe this one is ready.

@lukasmalkmus
Copy link
Contributor Author

@lukasmalkmus lukasmalkmus commented Jan 19, 2021

@matanw
Copy link

@matanw matanw commented Feb 2, 2021

It seems ready to merge, no? any reason no to merge it?

@neilotoole
Copy link

@neilotoole neilotoole commented Mar 13, 2021

Can we get this merged in plz?

jpmcb
jpmcb approved these changes May 3, 2021
Copy link
Collaborator

@jpmcb jpmcb left a comment

This looks good to me! - and thanks for adding tests, makes it easier to review!

@jpmcb jpmcb merged commit 6d00909 into spf13:master May 3, 2021
8 checks passed
@jpmcb jpmcb mentioned this pull request May 3, 2021
@lukasmalkmus lukasmalkmus deleted the completion-context branch May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants