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

misc #58

Merged
merged 4 commits into from
Nov 28, 2015
Merged

misc #58

merged 4 commits into from
Nov 28, 2015

Conversation

casperdcl
Copy link
Sponsor Member

  • safer close() methods in tests
  • maxinterval handled properly
  • docs
  • structure & formatting

@casperdcl
Copy link
Sponsor Member Author

still needs fixing for py3:

File "/home/travis/build/tqdm/tqdm/tqdm/tests/tests_tqdm.py", line 469, in test_close

@codecov-io
Copy link

Current coverage is 100.00%

Merging #58 into master will not affect coverage as of d759d77

@@            master     #58   diff @@
======================================
  Files            4       4       
  Stmts          212     217     +5
  Branches        46      46       
  Methods          0       0       
======================================
+ Hit            212     217     +5
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of d759d77

Powered by Codecov. Updated on successful CI builds.

@casperdcl casperdcl added p0-bug-critical ☢ Exception rasing p3-enhancement 🔥 Much new such feature labels Nov 28, 2015
@casperdcl casperdcl changed the title Bugfixes misc Nov 28, 2015
@casperdcl
Copy link
Sponsor Member Author

Any idea how to fix this? The closing() syntax doesn't seem to work on tqdm() itself on python3... Do we need to define our own __enter__() and __exit__() or is there a more elegant solution?

@casperdcl casperdcl added the help wanted 🙏 We need you (discussion or implementation) label Nov 28, 2015
@casperdcl
Copy link
Sponsor Member Author

one workaround is

from contextlib import contextmanager

@contextmanager
def closing(thing):
    try:
        yield thing
    finally:
        thing.close()

but that's not a proper fix... if people use the normal closing syntax in their own code (from contextlib import closing) it'll fail

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

Sorry, no idea, I didn't even know about closing() before!

@casperdcl
Copy link
Sponsor Member Author

Well in py2.7 the closing from contextmanager would do essentially what my comment above says. apparently this was deprecated in py3:
https://docs.python.org/2/library/contextlib.html

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

Yes, I think we need to define exit() and enter(). I think this is the cleanest, most pythonic approach. Why are you reluctant?

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

Ah yes indeed, this has been deprecated, but because we can use the with statement directly. But I guess the same problem will arise: without __exit__() nor __enter__(), Python won't know how to handle tqdm with the with statement...

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

To keep compatibility with Python 2.7, you can maybe do something like this:

try:
    with tqdm(_range(3)):
        pass
    closing = lambda x: x
except Exception:
    from contextlib import closing

Then you can do with closing(tqdm(...)) as progressbar whether it's Python 2 or 3 (I guess).

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

A good tutorial on how to define enter and exit (I don't know if the API changed since then, it's an old article): http://effbot.org/zone/python-with-statement.htm

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

Basically, it would be:

        def __enter__(self):
            # Nothing to initialize?
            return self
        def __exit__(self, type, value, traceback):
            self.close()

enhancement (all python versions) and bugfix (py3)
@casperdcl
Copy link
Sponsor Member Author

Hah I figured it out by reading all the docs for the different versions. Would've been quicker if I'd checked your new comments first 👍

@casperdcl casperdcl merged commit cf717e0 into master Nov 28, 2015
@casperdcl casperdcl deleted the bugfixes branch November 28, 2015 20:11
@casperdcl casperdcl removed the help wanted 🙏 We need you (discussion or implementation) label Nov 28, 2015
@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

Haha, yes this also happened to me, this is because you tried to fix this issue on your own (which is a quality actually) ;) Great job on this, this is way cleaner to write unit tests!

@casperdcl
Copy link
Sponsor Member Author

Yeah it was nice how aiming to clean up StringIO led to introducing a new feature for tqdm... Can you think of how we could use it in my urllib reporthook example?

@lrq3000
Copy link
Member

lrq3000 commented Nov 28, 2015

Um... I'm not sure it's possible to use that for the urllib example, because it uses a callback, and I don't think we can use the with syntax to enhance anything. The only thing that I can think of:

def my_hook(t):
    last_b = [0]

    def inner(b=1, bsize=1, tsize=None, close=False):
        if close:
            t.close()
            return
        t.total = tsize
        t.update((b - last_b[0]) * bsize) # manually update the progressbar
        last_b[0] = b
    return inner

eg_link = 'http://www.doc.ic.ac.uk/~cod11/matryoshka.zip'
with tqdm(unit='B', unit_scale=True, leave=True, miniters=1,
                  desc=eg_link.split('/')[-1]) as progressbar:
    eg_hook = my_hook(progressbar)
    # etc...

But this was also possible before (I just construct the tqdm object outside of the my_hook() function), so using with doesn't really add anything here... BUT, it's clearly more easy to instanciate a manually updateable progress bar like that (so that we now have two usage of tqdm: in for/while loops, and with with statement).

@casperdcl
Copy link
Sponsor Member Author

The new with syntax is better since you don't have to close explicitly... The example you gave above doesn't need the close=False and associated code anymore. I've updated it on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p0-bug-critical ☢ Exception rasing p3-enhancement 🔥 Much new such feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants