-
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
Extending redirection to stdout, stderr, stdin #822
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.
Fix up the comment on SetOutput so it's clear that SetOutput and SetOut are the same thing now and I'll merge this.
I kept the old behavior (backwards compatibility) and updated the comment to indicate the newer alternative methods. |
It would be very nice if this gets merged :) Testability of stdin is currently a huge issue. And as we know so far, stderr separation is also nice to have. |
@eparis We would love to get this merged as well so that we can migrate some of our tests to this format. |
@eparis Looks like this PR has some pretty broad support and a defined usecase! When do you anticipate this will be included in a release? 🙏 |
This looks good to me but would be nice to have some tests. |
I guess I said I'd merge this a long time ago. Anyone willing to commit to writing a unit test like @jharshman mentioned? |
@jharshman @eparis test cases have been added |
Thanks for adding some test cases. This looks good to me. |
Bump (gently) |
@jharshman @eparis @jamesdphillips All comments have been addressed |
(gently) Bump We'd appreciate if could review this again now that all comments have been addressed. We really need this feature to make our life easier and avoid dirty hacks while writing test cases for our sub-commands. As a last resort, we could fork cobra though we're committed to first try everything to avoid such duplication of code and effort. Thanks for considering. |
is there anything blocking this PR from being reviewed again ? |
@eparis bump (gently) :) |
Follow-up of spf13#822
Follow-up of spf13#822
*cobra.Command `SetOutput` method is deprecated [0]. Replacing with `SetOut` and `SetErr`. [0]: spf13/cobra#822
*cobra.Command `SetOutput` method is deprecated [0]. Replacing with `SetOut` and `SetErr`. [0]: spf13/cobra#822
A couple of issues in the past have already requested the ability of setting stdio (in, out, err): #763 #658
In addition, there are some discussions about testing cobra based applications #770
This PR tries to address this by allowing redirection of I/O at the command level allowing for better unit testing.
stdin
is also considered as some CLI may request data (such as passwords, confirmations) that may also need to be redirected when testing.