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

Replace clint by tqdm for progressbar #242

Merged
merged 1 commit into from
May 14, 2017

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Apr 27, 2017

Closes #241 – As clint as some indirect Python 2 dependency and tqdm has
not.

@@ -121,9 +121,14 @@ def _upload(self, package):
(package.basefilename, fp, "application/octet-stream"),
))
encoder = MultipartEncoder(data_to_send)
bar = ProgressBar(expected_size=encoder.len, filled_char='=')
bar = ProgressBar(total=encoder.len, unit='bytes', unit_scale=True, leave=False)
def update_progressbar(monitor, _cache=[0]):

Choose a reason for hiding this comment

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

This relies on a little Python quirk that isn't as intuitive. Thoughts on factoring this out into it's own object and then passing a method of that object to MultipartEncoderMonitor?

Copy link
Member

Choose a reason for hiding this comment

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

Why not use a variable that lives at the same scope as the function instead of relying on this. I agree with @SethMichaelLarson that this isn't going to be easy to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a custom subclass of tqdm with a update_to method.

setup.py Outdated
@@ -19,7 +19,7 @@


install_requires = [
"clint",
"tqdm",
Copy link
Member

Choose a reason for hiding this comment

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

Please add a lower limit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -121,9 +121,14 @@ def _upload(self, package):
(package.basefilename, fp, "application/octet-stream"),
))
encoder = MultipartEncoder(data_to_send)
bar = ProgressBar(expected_size=encoder.len, filled_char='=')
bar = ProgressBar(total=encoder.len, unit='bytes', unit_scale=True, leave=False)
def update_progressbar(monitor, _cache=[0]):
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a variable that lives at the same scope as the function instead of relying on this. I agree with @SethMichaelLarson that this isn't going to be easy to maintain.

setup.cfg Outdated
@@ -8,7 +8,7 @@ ignore =

[metadata]
requires-dist =
clint
tqdm
Copy link
Member

Choose a reason for hiding this comment

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

Can you have this match the version in install_requires. I'd really rather not use newer features of very recent versions of setuptools, e.g., ~=. Can we just use >=, please?

Copy link
Member

Choose a reason for hiding this comment

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

This needs to match setup.py. When people install from a wheel, we want the requirements to match those that they see when installing from an sdist.

"""
identical to update, except `n` should be current value and not delta.
"""
self.update(n-self.n)
Copy link
Member

Choose a reason for hiding this comment

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

I'm disappointed Flake8 didn't catch this, but can you add spaces around -?

@Carreau
Copy link
Contributor Author

Carreau commented Apr 29, 2017

Done and done. I did not realized that ~= was that recent as the pep 440 is from 2013.

@sigmavirus24
Copy link
Member

No worries. All I know is that even last year, setuptools didn't support it as well as it could. Plus setuptools is one of those things people don't ever consider upgrading unless they run into issues, so there's such a wide swath of versions that we need to avoid using features that are too new.

@sigmavirus24
Copy link
Member

There are still some Flake8 warnings that need cleaning up. https://travis-ci.org/pypa/twine/jobs/227188445#L201 You can find them locally with tox -e pep8. Thanks again for tackling this! 🍰

@Carreau
Copy link
Contributor Author

Carreau commented May 1, 2017

pep8ified.

@@ -13,7 +13,7 @@
# limitations under the License.
from __future__ import absolute_import, unicode_literals, print_function

from clint.textui.progress import Bar as ProgressBar
from tqdm import tqdm as tqdm

Choose a reason for hiding this comment

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

Pretty sure this has no effect. Should be:

from tqdm import tqdm

@@ -132,7 +142,7 @@ def _upload(self, package):
allow_redirects=False,
headers={'Content-Type': monitor.content_type},
)
bar.done()
bar.close()

Choose a reason for hiding this comment

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

Can tqdm.tqdm be used as a context manager? If so it would be nice to use it like one so that .close() is always called, even on exception.

Choose a reason for hiding this comment

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

If not it might be worth it to do something like this:

bar = ProgressBar(total=encoder.len, unit='bytes', unit_scale=True, leave=False)
try:
    # Setup monitor and response here.
finally:
    bar.close()

Copy link
Member

Choose a reason for hiding this comment

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

https://pypi.python.org/pypi/tqdm#manual leads me to believe that in fact tqdm can be used as a context manager.

Personally, I'd rather this look like it does except that we do:

with bar:
    resp = self.session.post(
    # ...
    )

Because I'd rather keep the scope of the context-manager small.

setup.cfg Outdated
@@ -8,7 +8,7 @@ ignore =

[metadata]
requires-dist =
clint
tqdm
Copy link
Member

Choose a reason for hiding this comment

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

This needs to match setup.py. When people install from a wheel, we want the requirements to match those that they see when installing from an sdist.

@@ -132,7 +142,7 @@ def _upload(self, package):
allow_redirects=False,
headers={'Content-Type': monitor.content_type},
)
bar.done()
bar.close()
Copy link
Member

Choose a reason for hiding this comment

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

https://pypi.python.org/pypi/tqdm#manual leads me to believe that in fact tqdm can be used as a context manager.

Personally, I'd rather this look like it does except that we do:

with bar:
    resp = self.session.post(
    # ...
    )

Because I'd rather keep the scope of the context-manager small.

@sigmavirus24
Copy link
Member

@Carreau for what it's worth, if this is more work than you'd like to complete, let me know and I'll add the final polishing touches for you.

@Carreau Carreau force-pushed the clint-to-tqdm branch 2 times, most recently from 18ce611 to 42e55e0 Compare May 2, 2017 17:22
Closes pypa#241 – As clint as some indirect Python 2 dependency and tqdm has
not.
@Carreau
Copy link
Contributor Author

Carreau commented May 2, 2017

@Carreau for what it's worth, if this is more work than you'd like to complete, let me know and I'll add the final polishing touches for you.

No it's ok, thanks for the proposal ! Now contexmanagerified, and setup.cfg fixed.

@sigmavirus24 sigmavirus24 merged commit 42e55e0 into pypa:master May 14, 2017
@sigmavirus24
Copy link
Member

Thanks @Carreau!! 🍰

casperdcl added a commit to tqdm/tqdm that referenced this pull request May 29, 2017
Class-based, inspired by [twine#242](pypa/twine#242),
[here](pypa/twine@42e55e06).
@casperdcl casperdcl mentioned this pull request May 29, 2017
7 tasks
@casperdcl casperdcl mentioned this pull request Apr 8, 2018
8 tasks
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.

3 participants