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

[WIP] Logging and more #55

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

[WIP] Logging and more #55

wants to merge 13 commits into from

Conversation

stbuehler
Copy link

As requested in #44.

I split it up into many single commit, some of them not breaking the API, not all of them needed for logging.

The ProgressReceiver part (and the final logging part) will break the API - maybe it would be better to have a separate "next" branch for those changes (I have some more ideas :D).

Windows not supported yet - maybe someone working on windows can help out.

@stbuehler
Copy link
Author

Now that I think of it, I didn't actually use the ProgressReceiver trait to implement logging - I only implemented logging for MultiBar, and used a separate "frontend" for the channel...

But it is possible to add a log method to the ProgressReceiver trait (I even added it in my first try, just didn't use it anywhere) and make it available through ProgressBar, although implementing Write on ProgressBar with it would conflict with the current semantics.

Should ProgressBar also support logging? How should the interface look like? Just a log method, or a Write implementation?

The current ("byte counting for progress") Write implementation for ProgressBar should imho be removed anyway, and be implemented on a wrapper type instead if needed.

@a8m
Copy link
Owner

a8m commented Jul 29, 2017

Thanks for your contribution @stbuehler ! before I review it, can you please add an example to the example directory, so I'll be able to run it?

@stbuehler
Copy link
Author

I'll try to come up with something simple.

Just to clarify: only the last commit actually adds the logging; the others before can be reviewed without the logging example :)

I also thought about using a similar patch without breaking the API. And then realized how broken the current implementation is:

  • finish* doesn't print a newline. Following data just happens to be on a new line because it fills the line - unless of course the width calculation broke, the terminal got resized, or someone tries to copy&paste from the terminal.
  • finish_print with MultiBar leaks resources (it doesn't count as "done" in MultiBar)

This could probably be fixed by printing newlines in PB, and removing them in MultiBar - but is it worth it to fix this?

@stbuehler
Copy link
Author

Hi, I just added a commit providing an example and added another fix to the list, and removed the Write implementation for ProgressBar.

I also worked on a branch which doesn't break the API, but is based on the same fixes (#57) as this one: https://github.com/stbuehler/rust-pbr/tree/logging-keep-api

Basically the API changes could be done in a separate "next version" branch, or be dropped completely.

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