Fall back to simple text output when stdout is not a TTY#1392
Fall back to simple text output when stdout is not a TTY#1392knQzx wants to merge 2 commits intoapple:mainfrom
Conversation
|
@knQzx Thank you for the PR! Could you please add the commands you tested with, and before/after output, to the PR description? Would it be possible to add an integration test for this? |
|
added before/after output to the description and an integration test for this |
|
@knQzx It was a long time ago, but I think we made an implementation decision during initial progress bar development that if stderr wasn't a tty, we would suppress progress output, so that you'd only see warnings and errors. I can think of an idea or two, but why is this change to include progress with the warnings and errors useful for you? |
|
@jglogan mainly for ci logs - when a pull runs in a pipeline and stderr goes to a file, it's hard to tell if things are progressing or stuck. what were the ideas you had in mind? |
|
@knQzx I think it's similar. The use case I was thinking about originally was, if run OTOH, if that command is long running and I want to do @dkovba Do you remember anything else from when you were developing the progress bar and we were talking about it? Looking at where we are with |
|
@knQzx Could you review https://github.com/apple/containerization/blob/main/CONTRIBUTING.md#submitting-issues-and-pull-requests and set up signing for your commits? They need to show up in the PR as Then the CI build can make progress. |
|
|
||
| class TestCLIProgressAuto: CLITest { | ||
| @Test func testAutoProgressFallsBackToPlainWhenPiped() throws { | ||
| do { |
There was a problem hiding this comment.
do-catch blocks in tests seem redundant.
| /// with `isatty`. If the output is a TTY, `.ansi` is used; otherwise | ||
| /// `.plain` is selected so that piped or redirected output still shows | ||
| /// simple line-by-line status text. | ||
| private func resolvedProgress(terminal: FileHandle = .standardError) -> ProgressType { |
There was a problem hiding this comment.
The terminal parameter is currently unused. Do we need it? Also, the default value .standardError contradicts the PR title mentioning stdout.
| do { | ||
| let (_, _, error, status) = try run(arguments: [ | ||
| "image", "pull", | ||
| "--progress", "none", |
There was a problem hiding this comment.
Consider adding the same test for --progress ansi.
|
Thank you for the contribution! 👍 |
summary
fixes #113
test plan
before (main):
after:
also added
TestCLIProgressAutointegration test covering auto/plain/none modes