-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: make InitDefaultCompletionCmd public #1467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I am not familiar with the details of the documentation generation code, this change does seem appropriate, and all the existing tests keep passing.
This PR is being marked as stale due to a long period of inactivity |
Random comment to make the bot not close this PR |
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 |
Right now calling |
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. |
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... |
This PR is being marked as stale due to a long period of inactivity |
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? |
@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 |
You're right - I was being overly cautious; thanks for the feedback all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, and if @gssbzn has the bandwidth, I'd like to see a bit of documentation around this now that we'll be making it a public method. Otherwise, we can address that in subsequent PRs
@jpmcb what kind of documentation were you looking for? when to use? would documenting in the different *.md files for |
I also took the liberty to directly call it when generating docs as it seems that was already the case for `InitDefaultHelpCmd` Merge spf13/cobra#1467 closes #1464
closes #1464
I also took the liberty to directly call it when generating docs as it seems that was already the case for
InitDefaultHelpCmd