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

add missing dependency 'requests' #374

Merged
merged 2 commits into from Sep 11, 2019
Merged

Conversation

@s-m-e
Copy link
Contributor

s-m-e commented Aug 5, 2019

v1.5 is missing a dependency in its requirements.txt: requests

It's being used here.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #374 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #374   +/-   ##
=======================================
  Coverage   89.94%   89.94%           
=======================================
  Files          35       35           
  Lines        5868     5868           
=======================================
  Hits         5278     5278           
  Misses        590      590
Flag Coverage Δ
#wradlibtests 89.94% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89dc3b4...14b91ac. Read the comment docs.

@kmuehlbauer

This comment has been minimized.

Copy link
Member

kmuehlbauer commented Sep 10, 2019

@s-m-e @egouden

I quickly looked over the code which uses the request package. From my perspective this can be switched to urllib quite easy. Then we would not need the requests package.

@egouden

This comment has been minimized.

Copy link
Collaborator

egouden commented Sep 11, 2019

It was just easier to use Requests which is a kind of standard.
But one could use urllib/urllib2 to limit the dependency list if we don't plan many other http requests in the future.

@kmuehlbauer

This comment has been minimized.

Copy link
Member

kmuehlbauer commented Sep 11, 2019

@egouden I see the point. So if using urllib is too tedious we should keep using requests. The only problem would be adding the custom header, right?

@kmuehlbauer

This comment has been minimized.

Copy link
Member

kmuehlbauer commented Sep 11, 2019

@egouden Thinking a bit more about it, I'm already convinced. requests is already in the minimal conda python, so we should not bother about it. I'll pull the changes in after merging #381 and rebase.

@s-m-e

This comment has been minimized.

Copy link
Contributor Author

s-m-e commented Sep 11, 2019

requests is the 7th most used pypi package. There is nothing wrong with having it as a dependency. It's shipped virtually everywhere - it's even included in the OSGeo4W windows distribution.

@kmuehlbauer

This comment has been minimized.

Copy link
Member

kmuehlbauer commented Sep 11, 2019

As already written, I'm convinced 😁 Waiting for #381...

@kmuehlbauer kmuehlbauer merged commit ae6aa24 into wradlib:master Sep 11, 2019
4 checks passed
4 checks passed
codecov/patch Coverage not affected when comparing 89dc3b4...14b91ac
Details
codecov/project 89.94% remains the same compared to 89dc3b4
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmuehlbauer kmuehlbauer added this to the 1.6 milestone Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.