Review suggestions#1
Conversation
cli/command/container/cp.go
Outdated
| } | ||
|
|
||
| fmt.Fprint(dockerCli.Err(), "Preparing to copy...\n") | ||
| fmt.Fprint(dockerCli.Err(), "\033[s") |
There was a problem hiding this comment.
This should probably be replaced with a aec.XX. I wasn't sure what code this was
There was a problem hiding this comment.
This cursor command (\033[s) saves the cursor position, in this case, the beginning the new line.
There was a problem hiding this comment.
I think that aec.save() could solve this.
There was a problem hiding this comment.
Hello @thaJeztah, how are you? Did you see this change in this line?
There was a problem hiding this comment.
oh! missed it (or thought you would include it in after this was merged); let me update
|
First of all, thanks for your comments and PR. I'll check all the points. |
maximillianfx
left a comment
There was a problem hiding this comment.
Good points, I've made some observations and questions, thanks!
cli/command/container/cp.go
Outdated
| } | ||
|
|
||
| fmt.Fprint(dockerCli.Err(), "Preparing to copy...\n") | ||
| fmt.Fprint(dockerCli.Err(), "\033[s") |
There was a problem hiding this comment.
This cursor command (\033[s) saves the cursor position, in this case, the beginning the new line.
- remove fields from cpWithProgress that were not used
- only wrap cpWithProgress when printing progress
- rename cpWithProgress to copyProgressReader
- use aec for terminal control sequences
- print progress to STDERR instead of STDOUT
- don't use terminal detection to set the default, and instead perform
it if no value was set. This prevents confusion when doing, e.g.:
docker cp --help | grep ...
- return early if "quiet", to simplify multiple "if" statements
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
f8e1f92 to
fb38ac4
Compare
|
@maximillianfx updated; PTAL |
|
LGTM, I'll made some tests here to see if breaks something. |
Opening my branch with suggested changes as a pull-request (see docker#2708 (review))
it if no value was set. This prevents confusion when doing, e.g.:
docker cp --help | grep ...