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
Refactor progress bars #5763
Refactor progress bars #5763
Conversation
30d335e
to
541ccff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very, very good!
This refactor is very challenging, but excellent job here.
Please have a look to the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking a bit, and I would say that the correct approach would be:
- Do not change the
output.py
to include tqdm stuff - Make an Adapter around an output, that returns a similar output object, but that overwrites the
write
with the custom tqdm stuff. This adapter is part of theprogress_bar
package - Pass adaptors around the outputs to those who need them in the context of progress bars being displayed. They shouldn't be many, I'd say that only 1 or 2 different messages are interleaved to progress bars.
This is just an idea, lets wait for @lasote feedback on this PR.
I like that idea and would be much safer than using it for all the output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I talked with you on Slack, probably we can unify ReadProgress
and WriteProgress
.
I've been trying locally and works very well. The code is much nicer too. So great work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question, but otherwise fantastic job, can be merged (after fixing the broken iterator: https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-5763/37/pipeline)
Changelog: Feature: Refactor Conan Upload, Download and Compress progress bars.
Docs: omit
Closes #5739
develop
branch, documenting this one.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.