Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAvoid printing header if piped to a script #796
Conversation
|
Thanks! For now I'd rather see us stick with the |
|
Thank you for updates! |
| @@ -264,7 +264,7 @@ Requesting a code review from you | |||
| } | |||
| } | |||
|
|
|||
| func TestPRList(t *testing.T) { | |||
| func TestPRList_stdout_is_not_a_tty(t *testing.T) { | |||
This comment has been minimized.
This comment has been minimized.
mislav
Apr 21, 2020
Member
The updated test name makes me think that we should really have a way to force "tty mode" in tests to be able to assert such output.
This is not something that you have to do in this PR, btw. I don't know yet how would we pass that information to tests.
| @@ -278,17 +278,14 @@ func TestPRList(t *testing.T) { | |||
| t.Fatal(err) | |||
| } | |||
|
|
|||
| eq(t, output.Stderr(), ` | |||
| Showing 3 of 3 pull requests in OWNER/REPO | |||
This comment has been minimized.
This comment has been minimized.
mislav
Apr 21, 2020
Member
I would recommend that we hold off merging this until we figure out how to simulate tty mode for tests so we can keep asserting this output. The header is an important part of output and I wish we can keep test coverage of it.
This comment has been minimized.
This comment has been minimized.
vilmibm
Apr 28, 2020
Member
I think that is fair. In reviewing #572 and then returning to this I think we need to step back and design a holistic approach to determining what kind of output (both formatting and color) to use; something routed via the cmd reference that can be stubbed appropriately when running commands in a test.
| // TODO: avoid printing header if piped to a script | ||
| fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) | ||
|
|
||
| if utils.IsTerminal(os.Stdout) { |
This comment has been minimized.
This comment has been minimized.
mislav
Apr 21, 2020
Member
Instead of using global os.Stdout, we reference the output stream inside commands as cmd.OutOrStdout(), so you should be checking that object instead. This gives us the ability to override the output stream for tests. Note that the output stream is a Writer and not necessarily a File, so you would have to do an additional check.
Tarasovych commentedApr 17, 2020
Fixes TODO