cobra icon indicating copy to clipboard operation
cobra copied to clipboard

feat: make InitDefaultCompletionCmd public

Open gssbzn opened this issue 3 years ago • 10 comments

closes #1464

I also took the liberty to directly call it when generating docs as it seems that was already the case for InitDefaultHelpCmd

gssbzn avatar Aug 03 '21 11:08 gssbzn

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 03 '21 11:08 CLAassistant

This PR is being marked as stale due to a long period of inactivity

github-actions[bot] avatar Oct 19 '21 00:10 github-actions[bot]

Random comment to make the bot not close this PR

gssbzn avatar Oct 28 '21 08:10 gssbzn

Hmmm I want to take a second to think about this one quick before merging.

We'd be changing the completions API to include a new Init that was once private but not changing the existing API that would constitute a breaking change?

jpmcb avatar Dec 07 '21 22:12 jpmcb

but not changing the existing API that would constitute a breaking change?

Right now calling cmd.InitDefaultCompletionCmd() inside the different Gen commands could be seen as a breaking change I could give you that, I'm happy to rollback that change if that gets this merged, at least having InitDefaultCompletionCmd public would reduce a lot of the hacking we have in place to be able to generate docs for the auto complete commands

gssbzn avatar Dec 08 '21 12:12 gssbzn

This all looks good to me, but I think just for stability, this needs to wait for a 2.0 release. Since technically the API change would be a breaking change. If there's one thing I hate, it's surprising users with unexpected breaking changes 😂

Otherwise, I think this implementation makes perfect sense.

jpmcb avatar Dec 08 '21 15:12 jpmcb

We'd be changing the completions API to include a new Init that was once private but not changing the existing API that would constitute a breaking change?

I'm not understanding well. Adding a new public function does not break the API, does it? We've added a bunch of public functions and constants when we added Go shell completions...

marckhouzam avatar Dec 08 '21 16:12 marckhouzam

This PR is being marked as stale due to a long period of inactivity

github-actions[bot] avatar Feb 07 '22 00:02 github-actions[bot]

The exported method itself shouldn't be breaking unless I'm misunderstanding something. @jpmcb if he removes the addition to the method and just exports the method, is this good to merge?

johnSchnake avatar Feb 18 '22 16:02 johnSchnake

@marckhouzam @jpmcb I've removed the calls to document the completion command from the default docs in the hope that this change makes it for the spring release, let me know if there's something else of concern here

gssbzn avatar May 04 '22 15:05 gssbzn

I'm not understanding well. Adding a new public function does not break the API, does it? We've added a bunch of public functions and constants when we added Go shell completions...

You're right - I was being overly cautious; thanks for the feedback all

jpmcb avatar Oct 04 '22 22:10 jpmcb

@jpmcb what kind of documentation were you looking for? when to use? would documenting in the different *.md files for /doc enough?

gssbzn avatar Oct 05 '22 15:10 gssbzn