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

Remove extra output in bash completion #592

Merged
merged 2 commits into from
Sep 13, 2022
Merged

Conversation

praveenrewar
Copy link
Member

What this PR does / why we need it:

  • Do not print Succeeded for help and hidden __complete command
  • Use SetOut instead of deprecated SetOutput for setting output for help command.

Which issue(s) this PR fixes:

Fixes #583

Does this PR introduce a user-facing change?


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


Currently we are using SetOutput which also sets the errWriter and is also deprecated
@praveenrewar praveenrewar force-pushed the bash-completion branch 2 times, most recently from 9ba9ef3 to 971a987 Compare August 25, 2022 11:01
cmd/kapp/kapp.go Outdated Show resolved Hide resolved
@cppforlife
Copy link
Contributor

why do we need to special case this? wouldnt all of these commands (case "help", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd) be running in non-tty env, so Succeeded shouldnt be showing up already?

@praveenrewar
Copy link
Member Author

why do we need to special case this? wouldnt all of these commands (case "help", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd) be running in non-tty env

These commands are a bit special, quoting from the issue itself:

This command is added by Cobra when rootCmd.Execute() is called, which makes it a little trickier to adjust the behaviour for it.

So, right now when we decide which commands should have the default tty behaviour and which should not have it, these commands are not even added to the command tree 😞

@rohitagg2020
Copy link
Contributor

LGTM

Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

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

The cobrautil path looks much cleaner!
LGTM 🚀

@praveenrewar praveenrewar merged commit ed8c3f2 into develop Sep 13, 2022
@praveenrewar praveenrewar deleted the bash-completion branch September 13, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra output and errors during shell completion
6 participants