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

Improve download function #118

Merged
merged 5 commits into from
May 16, 2015
Merged

Improve download function #118

merged 5 commits into from
May 16, 2015

Conversation

vdumoulin
Copy link
Contributor

This PR adds a progress bar. Partially fixes #117.

@vdumoulin
Copy link
Contributor Author

@dwf

@@ -11,6 +12,22 @@ class NeedURLPrefix(Exception):
pass


@contextmanager
def progress_bar(name, maxval):
Copy link
Contributor

Choose a reason for hiding this comment

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

nice use of context managers

@vdumoulin
Copy link
Contributor Author

I'm debating adding an additional dependency (httmock) to mock HTTP requests. It allows to speed up the tests as well as craft responses that target specific cases in download helpers. Any thoughts?

@dwf
Copy link
Contributor

dwf commented May 16, 2015

Probably a good call (running tests offline is kind of important for working on the road, and the UCI people probably do not appreciate our Travis bot hitting their server frequently), but save that for another PR.

dwf added a commit that referenced this pull request May 16, 2015
@dwf dwf merged commit a4a09d3 into mila-iqia:master May 16, 2015
@bartvm
Copy link
Member

bartvm commented May 16, 2015

For tests you can add whatever you want I think, because the user won't
have to install them. Just make sure to add them to req-travis-pip.txt
after #120 got merged

On Sat, May 16, 2015 at 6:06 PM, vdumoulin notifications@github.com wrote:

I'm debating adding an additional dependency (httmock) to mock HTTP
requests. It allows to speed up the tests as well as craft responses that
target specific cases in download helpers. Any thoughts?


Reply to this email directly or view it on GitHub
#118 (comment).

@bartvm
Copy link
Member

bartvm commented May 16, 2015

Oh, and add them as an optional dependency in setup.py

@dwf
Copy link
Contributor

dwf commented May 16, 2015

IIRC there's some sort of "tests_require" thing in setup.py, no?

On Sat, May 16, 2015 at 6:08 PM, Bart van Merriënboer <
notifications@github.com> wrote:

Oh, and add them as an optional dependency in setup.py


Reply to this email directly or view it on GitHub
#118 (comment).

@bartvm
Copy link
Member

bartvm commented May 16, 2015

@vdumoulin vdumoulin deleted the download_fn branch May 16, 2015 23:22
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.

More robust download function
3 participants