Skip to content

Revert PR #113 #123

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

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Revert PR #113 #123

merged 2 commits into from
Jul 19, 2022

Conversation

bodgit
Copy link
Contributor

@bodgit bodgit commented Jul 19, 2022

See my comment here: #108 (comment)

The original PR is/was wrong in that it mixes up os.Stdout and os.Stderr which might appear to produce the correct behaviour until you use some shell redirection to send one of those two outputs somewhere other than the controlling terminal.

If the progress bar is writing to os.Stderr then that final newline should also be sent to the same destination; the code was originally correct.

This PR reverts the original commit made in #113 but before I did that, I updated the two tests that use DefaultBytes() to ensure that nothing ever gets written to os.Stdout by the progress bar code. These tests obviously fail initially, but then pass with the reverted commit.

bodgit added 2 commits July 19, 2022 17:37

Verified

This commit was signed with the committer’s verified signature.
bodgit Matt Dainty

Verified

This commit was signed with the committer’s verified signature.
bodgit Matt Dainty
This reverts commit 33cab3a.
@5HT2
Copy link
Contributor

5HT2 commented Jul 19, 2022

Oh, thank you. I appreciate someone fixing my issue properly lol.

@schollz
Copy link
Owner

schollz commented Jul 19, 2022

thanks! I appreciate the fix @bodgit

@schollz schollz merged commit 4ded600 into schollz:master Jul 19, 2022
@bodgit bodgit deleted the revert-113 branch July 22, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants