Replace clint by tqdm for progressbar #242

Carreau · 2017-04-27T21:05:26Z

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

sethmlarson · 2017-04-27T22:27:41Z

twine/repository.py

@@ -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]):


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?

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.

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

sigmavirus24 · 2017-04-27T22:35:34Z

setup.py

@@ -19,7 +19,7 @@


 install_requires = [
-    "clint",
+    "tqdm",


Please add a lower limit here.

sigmavirus24 · 2017-04-27T22:37:15Z

twine/repository.py

@@ -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]):


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.

sigmavirus24 · 2017-04-28T22:51:07Z

setup.cfg

@@ -8,7 +8,7 @@ ignore =

 [metadata]
 requires-dist =
-    clint
+    tqdm


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?

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.

sigmavirus24 · 2017-04-28T22:51:43Z

twine/repository.py

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


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

Carreau · 2017-04-29T17:58:28Z

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

sigmavirus24 · 2017-04-29T21:55:15Z

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 · 2017-04-29T22:30:54Z

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 · 2017-05-01T15:39:38Z

pep8ified.

sethmlarson · 2017-05-01T18:13:42Z

twine/repository.py

@@ -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


Pretty sure this has no effect. Should be:

from tqdm import tqdm

sethmlarson · 2017-05-01T18:15:09Z

twine/repository.py

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


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.

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()

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 · 2017-05-02T12:39:38Z

setup.cfg

@@ -8,7 +8,7 @@ ignore =

 [metadata]
 requires-dist =
-    clint
+    tqdm


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.

sigmavirus24 · 2017-05-02T12:41:32Z

twine/repository.py

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


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 · 2017-05-02T12:42:32Z

@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.

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

Carreau · 2017-05-02T17:23:14Z

@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 · 2017-05-14T16:29:35Z

Thanks @Carreau!! 🍰

Class-based, inspired by [twine#242](pypa/twine#242), [here](pypa/twine@42e55e06).

Carreau mentioned this pull request Apr 27, 2017

Twine have indirect Python 2 only dependency. #241

Closed

sethmlarson reviewed Apr 27, 2017

View reviewed changes

sigmavirus24 requested changes Apr 27, 2017

View reviewed changes

sigmavirus24 requested changes Apr 28, 2017

View reviewed changes

Carreau force-pushed the clint-to-tqdm branch from 38e3169 to f3b7492 Compare April 29, 2017 17:56

Carreau force-pushed the clint-to-tqdm branch from f3b7492 to 03bf142 Compare May 1, 2017 15:28

sethmlarson reviewed May 1, 2017

View reviewed changes

sigmavirus24 requested changes May 2, 2017

View reviewed changes

Carreau force-pushed the clint-to-tqdm branch 2 times, most recently from 18ce611 to 42e55e0 Compare May 2, 2017 17:22

Replace clint by tqdm for progressbar

42e55e0

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

sigmavirus24 merged commit 42e55e0 into pypa:master May 14, 2017

casperdcl added a commit to tqdm/tqdm that referenced this pull request May 29, 2017

update to OOP urllib hook example

c1d56bf

Class-based, inspired by [twine#242](pypa/twine#242), [here](pypa/twine@42e55e06).

casperdcl mentioned this pull request May 29, 2017

update and tweak tqdm progressbar #256

Merged

7 tasks

casperdcl mentioned this pull request Apr 8, 2018

recognition tqdm/tqdm#247

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace clint by tqdm for progressbar #242

Replace clint by tqdm for progressbar #242

Carreau commented Apr 27, 2017

sethmlarson Apr 27, 2017

sigmavirus24 Apr 27, 2017

Carreau Apr 27, 2017

sigmavirus24 Apr 27, 2017

Carreau Apr 27, 2017

sigmavirus24 Apr 27, 2017

sigmavirus24 Apr 28, 2017

sigmavirus24 May 2, 2017

sigmavirus24 Apr 28, 2017

Carreau commented Apr 29, 2017

sigmavirus24 commented Apr 29, 2017

sigmavirus24 commented Apr 29, 2017

Carreau commented May 1, 2017

sethmlarson May 1, 2017

sethmlarson May 1, 2017

sethmlarson May 1, 2017

sigmavirus24 May 2, 2017

sigmavirus24 May 2, 2017

sigmavirus24 May 2, 2017

sigmavirus24 commented May 2, 2017

Carreau commented May 2, 2017

sigmavirus24 commented May 14, 2017

Replace clint by tqdm for progressbar #242

Replace clint by tqdm for progressbar #242

Conversation

Carreau commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carreau commented Apr 29, 2017

sigmavirus24 commented Apr 29, 2017

sigmavirus24 commented Apr 29, 2017

Carreau commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigmavirus24 commented May 2, 2017

Carreau commented May 2, 2017

sigmavirus24 commented May 14, 2017