Check if specified raster size exists before trying to download it #47
Conversation
|
To be more robust you might sort them by size before picking the largest. (But the first one is the largest, yes, and I can't think of a reason that would change.) |
There was a problem hiding this comment.
Nothing to add of any value here. All looks great to me! Great job @willgearty. I only have one tiny comment if I'm being picky but it doesn't really matter.
| ind <- grepl(format, rasters$sizes) | ||
| if (!any(ind)) { | ||
| ind <- 1 | ||
| warning(paste("No raster image with dimension", format, "available.", |
There was a problem hiding this comment.
| warning(paste("No raster image with dimension", format, "available.", | |
| warning(paste("No raster image with dimensions", format, "available.", |
Here you use dimension but dimensions in L54. The same in L43 and L44.
There was a problem hiding this comment.
Ah, so those spellings are/were intentional because the first value in each message is just a single number (e.g., "512"), hence "dimension", whereas the second number in each message is two values (e.g., "512x300"), hence "dimensions"
There was a problem hiding this comment.
That makes absolute sense then. Do you think this could be potentially confusing for a user though? After receiving such a warning, they might want to update their script to use the specific dimension to prevent getting warnings? Should they always use the first value? If so, maybe we should just return this?
There was a problem hiding this comment.
I thought about doing that, but sometimes the phylopic is landscape, so the larger number (which I believe is always the format value) isn't always the first value in the dimensions (e.g., "300x512"). I suppose we could do some string manipulation to extract the larger of the two numbers? But that seemed like more work than it was worth at the time.
There was a problem hiding this comment.
I see! Yes, that's a bit trickier. If the format value is always the larger value then it should be relatively straightforward as you've suggested. I don't think it is a huge issue as it is though... I can just imagine a few puzzled faces every now and then. I'll let you decide!
There was a problem hiding this comment.
So, I was about to implement this, but then noticed that if the only available size is smaller than the options we currently provide (in the case of c8f71c27-71db-4b34-ac2d-e97fea8762cf, the largest size raster is 210x143), then the user won't be able to supply that format to get_phylopic as the function is currently implemented.
We could expand the format options to any number (and then pick the closest size, maybe?), but I feel like that opens a whole new can of worms that I think is outside of the scope of this PR.
This is a small fix for edge cases where the specified raster size doesn't exist on phylopic. In these cases, we now return whatever is the first raster (which I believe is usually the largest one) and also display a warning.
Fixes #46.