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

Default to printing progress bar to sterr, rather than stdout. #47

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

InnovativeInventor
Copy link
Contributor

As per #46, the correct default behavior of this package should be to print to stderr. This commit brings us in-line with other ProgressBar-style utilities, like pv and tqdm. It also allows ProgressBar.jl to be dropped into piped scripts.

With this change, ProgressBar will print to stderr by default. In a normal terminal, ProgressBar will look the same as it used to. To set ProgressBar to print to stdout and behave as it used to, ProgressBar now accepts an output_stream argument, which can be set to stdout. E.g.:

ProgressBar(1:5, output_stream=stdout)

As per cloud-oak#46, the
correct default behavior of this package should be to print to
stderr. This commit brings us in-line with other ProgressBar-style
utilities, like `pv` and `tqdm`. It also allows ProgressBar.jl to
be dropped into piped scripts.

By default, ProgressBar will print to stderr. In a normal terminal,
ProgressBar will look the same as it used to. To set ProgressBar to
print to stdout and behave as it used to, ProgressBar now accepts a
`output_stream` argument, which can be set to `stdout`. E.g.:
`ProgressBar(1:5, output_stream=stdout)`
Copy link
Member

@khdlr khdlr left a comment

Choose a reason for hiding this comment

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

Thanks, looks pretty good already!

In the current version, stderr is hardcoded in some places where we should use t.output_stream instead.

src/ProgressBars.jl Outdated Show resolved Hide resolved
src/ProgressBars.jl Outdated Show resolved Hide resolved
src/ProgressBars.jl Outdated Show resolved Hide resolved
@InnovativeInventor
Copy link
Contributor Author

InnovativeInventor commented Jul 27, 2021

🤦 I feel silly -- totally forgot that I added those when testing! Thanks for catching this!

@khdlr
Copy link
Member

khdlr commented Jul 27, 2021

Cool, thanks for the quick update! Looks good to me now, will merge and push a new version to juliahub soon 👍

@InnovativeInventor
Copy link
Contributor Author

No need to rush, but I'm wondering when you'll push a new version to juliahub. A package that I'm maintaining (GerryChain Julia) is going to be receive a release soon, and it'd be great if we could include progress bars in our next release (no worries if not, though).

@khdlr khdlr merged commit 0f7ee3e into cloud-oak:master Aug 12, 2021
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