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

cli: revert format() related changes #7840

Merged

Conversation

xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Mar 10, 2020

Since the progress bar API is % related, it seems simpler to stick with
it.
Fix #6973 (comment) after #7826

Since the progress bar API is % related, it seems simpler to stick with
it.
@xavfernandez xavfernandez added the skip news Does not need a NEWS file entry (eg: trivial changes) label Mar 10, 2020
@xavfernandez
Copy link
Member Author

An alternative would be to provide an updated update method to BaseDownloadProgressBar.

@jaraco
Copy link
Member

jaraco commented Mar 10, 2020

I apologize for not getting this right. I guess it wasn't covered by the tests. I'd be happy to provide a .format fix instead (and test it or consider writing tests) if there's interest in having the pip codebase be pure w.r.t. % formatters.

@atugushev
Copy link
Contributor

@xavfernandez thanks for the fix! Works fine now.

@pradyunsg pradyunsg merged commit e15ef59 into pypa:master Mar 10, 2020
@pradyunsg
Copy link
Member

I apologize for not getting this right.

No worries -- we're all human!

I'd be happy to provide a .format fix instead (and test it or consider writing tests) if there's interest in having the pip codebase be pure w.r.t. % formatters.

This, would be awesome!

@xavfernandez
Copy link
Member Author

I apologize for not getting this right. I guess it wasn't covered by the tests. I'd be happy to provide a .format fix instead (and test it or consider writing tests) if there's interest in having the pip codebase be pure w.r.t. % formatters.

@jaraco no need to apologize, we now know we are lacking tests on this part and an other fix with tests would be most welcome 👍

@xavfernandez xavfernandez deleted the revert_progress_bar_format_change branch March 10, 2020 20:57
@xavfernandez
Copy link
Member Author

@atugushev you're welcome ! Thanks for eagerly testing the master branch :)

@jaraco
Copy link
Member

jaraco commented Mar 11, 2020

This, would be awesome!

I see now that the vendored package is built based on % formatters; what you meant by:

Since the progress bar API is % related, it seems simpler to stick with it.

I agree.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Apr 15, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update %-format call to use str.format
4 participants