Skip to content

imread with URL string unit test #4485

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

Closed
wants to merge 8 commits into from

Conversation

rnelsonchem
Copy link
Contributor

This PR adds a test case to #4256. At this point, it may need some careful thought because it pulls an image down from the internet. This may be a violation of Debian's packaging rules. See #4256 for more discussion.

These modifications allow valid URL strings to be passed directly into imread.
URL recognition is done using the standard library function urlparse. If the
string contains a valid url scheme, it will be used to download and process
the data using a combination of urlopen and BytesIO (StringIO in python 2).
The Pillow docs state that Image.open can handle either a filename string or a
file object. That simplifies the pilread function substantially.
It seems that the identity test for the parsed URL scheme was not sufficient.
I changed this to an equality test, and things are now working properly.
Also added the logo2.png file to the test image folder. If there is a
URLError, imread will simply open the local file and print a little error.
@rnelsonchem
Copy link
Contributor Author

@tacaswell I tried to add the test case as a new branch at the end of my original PR. But now it looks like this brought in all of the commits from my other branch as well. I hope this is okay. If not, could you provide some guidance on how to fix this.

@tacaswell
Copy link
Member

Just leave it, github is very clever and this will look better once the other PR is merged.

@tacaswell tacaswell added this to the proposed next point release milestone Jun 16, 2015
@WeatherGod
Copy link
Member

Anything holding up this PR?

@tacaswell
Copy link
Member

The hold up on this is ensuring that adding a test that touches the internet is kosher with debian packaging rules.

@QuLogic
Copy link
Member

QuLogic commented Oct 23, 2016

Could we perhaps use http.server/SimpleHTTPServer for the test instead?

@tacaswell tacaswell modified the milestone: 2.1 (next point release) Aug 29, 2017
@anntzer anntzer mentioned this pull request Mar 17, 2018
6 tasks
@anntzer anntzer closed this Mar 17, 2018
@anntzer
Copy link
Contributor

anntzer commented Mar 17, 2018

Replaced by #10817.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants