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

Create photoUrl for original size photos correctly #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jerbeers
Copy link

@jerbeers jerbeers commented May 8, 2014

See note on original size photos here:
https://www.flickr.com/services/api/misc.urls.html

@devedup
Copy link
Owner

devedup commented May 24, 2014

I've had a look at the code but I need to test this. I see on line 418 you default to jpg, but then you request the originalExtension from the photo dictionary on line 423 - which will only be there if it was added to the extras in the request. This means if you don't add it, then this line will return nil and you then have a nil extension.

Also, why even allow the user of to specify the extension as a param?... can an original photo be uploaded in multiple formats? If not, then why not either default to jpg, or look up what the actual extension is if its in the request dictionary and keep it simple?

What happens if I request the wrong extension?

@jerbeers
Copy link
Author

The way I understand it, if you upload a photo of a different file format than jpg, Flickr will create all the other size photos as jpg, but the original is the original file format. The default of jpg is to keep the current state (see deleted line 441 that hard-codes jpg). Yes, you have to request the extras, but you could assert to validate that the keys are present. But even with that assumption, it's equal or better to what it does now, which is return a url for original size that doesn't work.

I added the extension parameter to allow the existing method signature to stay the same. Perhaps that method shouldn't be in the header but just an internal method. If you request the wrong extension, you'll get the same as you get now, a URL that returns a "not found" image.

Thanks for the review!

@devedup
Copy link
Owner

devedup commented May 24, 2014

Ok that all makes sense. What I'll do is merge yours in (to give you credit where it's due) and then I'll make a few improvements on what you've started to make it a little more robust.

Thanks

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.

2 participants