Skip to content
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

Write output to stderr instead of stdout #1

Merged
merged 2 commits into from
Dec 20, 2020

Conversation

cole-miller
Copy link
Contributor

@cole-miller cole-miller commented Dec 19, 2020

stderr is appropriate for dynamic output that's not intended to be machine-readable.

Even better on POSIX systems would be writing to /dev/tty, but I'm not sure what the cross-platform equivalent is there.

@fosskers
Copy link
Owner

Ah right, the other libs do this too. Let me do some quick tests.

@fosskers
Copy link
Owner

fosskers commented Dec 19, 2020

This seems to make the cursor very excited, and the output is much less fluid.

Peek 2020-12-19 09-12

I used to have the cursor problem on master until I added a \r to the end of each print! statement. Any thoughts?

@cole-miller
Copy link
Contributor Author

Hmm, I bet it's some kind of buffering issue.

@fosskers
Copy link
Owner

In both alacritty and xterm, the animation fluidity slows down as time passes.

@cole-miller
Copy link
Contributor Author

I seem to have fixed this issue, commit coming shortly...

@cole-miller
Copy link
Contributor Author

Wrapping Stderr in a LineWriter and replacing all print! and println! with write!(&mut self.out, ...) and writeln!(&mut self.out, ...) (which I should've done initially :P) does the trick. I've also updated the examples to write everything to stderr.

One issue I noticed while testing is that terminal_size determines whether to return None by checking whether stdout is a terminal, so if you do

$ cargo run --example multi >/dev/null

then the output will be screwed up even though stderr is a perfectly good terminal. I don't see how to fix that downstream, though.

@fosskers
Copy link
Owner

Indeed that fixes it. I had assumed that someone would mention write! eventually, so thanks for that. I'll look over things carefully and merge this tonight.

This is more appropriate for dynamic output that's not intended to be
machine-readable.

`std::io::Stderr` is unbuffered by default, so add explicit
line-buffering to prevent the terminal cursor from jumping around.

Also update the examples to write their messages to stderr as well, for
consistency.
"{:<l$} [{:->f$}] 0%",
label,
"",
l = twidth - w - 8 - 5,
f = w
);
self.out.flush().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flush was removed here, but kept elsewhere. What was the intent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we call writeln! and self.out is line-buffered it'll flush anyway. The other cases don't write a newline so an explicit flush is needed.

@@ -9,7 +9,7 @@ use std::sync::{Arc, Mutex};
use std::time::Duration;

fn main() {
println!("Starting bars...");
eprintln!("Starting bars...");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can stay as println!, they're just demos after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to keep them that way? eprintln seems more consistent to me, but I'll defer to your judgement.

@fosskers fosskers merged commit 40d9a47 into fosskers:master Dec 20, 2020
@fosskers
Copy link
Owner

Thanks for this! Releasing a new version right away.

@cole-miller cole-miller deleted the stderr branch December 20, 2020 04:24
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.

2 participants