Conversation
relies on Image Process pre-release; see pelican-plugins/image-process#100
There was a problem hiding this comment.
Thanks for the PR. This is a helpful improvement that makes the plugin easier to use.
However your PR is incomplete. You have modified the most common usage (process_img_tag()). You should also make sure other cases adopt the same behavior and handle the exceptions raised by process_picture(). The functions build_srcset(), convert_div_to_picture_tag(), process_picture() and process_metadata() would all need to be modified to make the new behavior happen in all cases.
We also should have tests for this new behavior.
| logger.warning( | ||
| f'{LOG_PREFIX} Source image "{path}" is not supported by Pillow.' | ||
| ) | ||
| logger.info(f'{LOG_PREFIX} Source image "{path}" is not supported by Pillow.') |
There was a problem hiding this comment.
| logger.info(f'{LOG_PREFIX} Source image "{path}" is not supported by Pillow.') | |
| logger.info(f'{LOG_PREFIX} Source image "{path}" will not be processed because it is not supported by Pillow.') |
| except (UnidentifiedImageError, FileNotFoundError): | ||
| return None | ||
| except (UnidentifiedImageError, FileNotFoundError): # noqa: TRY203 | ||
| # return None |
There was a problem hiding this comment.
| # return None |
| img["srcset"] = ", ".join(srcset) | ||
|
|
||
|
|
||
| def convert_div_to_picture_tag(soup, img, group, settings, derivative): |
There was a problem hiding this comment.
This function should be modified so that original images are used if process_image() fails. I think the correct behavior in this case would be to use the original images in the <picture>.
| return f"{path} {condition}" | ||
|
|
||
|
|
||
| def build_srcset(img, settings, derivative): |
There was a problem hiding this comment.
This function should be modified to behave gracefully when process_image raises an exception. I think the correct behavior would be to use the original image for src and to not add the srcset attribute.
| img.wrap(picture_tag) | ||
|
|
||
|
|
||
| def process_picture(soup, img, group, settings, derivative): |
There was a problem hiding this comment.
This function should be modified so that original images are used if process_image() fails. I think the correct behavior in this case would be to use the original images in the <picture>.
| path = compute_paths(value, generator.context, derivative) | ||
|
|
||
| original_values[key] = value | ||
| metadata[key] = urljoin( |
There was a problem hiding this comment.
Image paths in metadata should not be changed if process_images raises an exception.
|
Ok, I'll work on this, but it will take a while to work through, as I don't think I've used anything other that |
2 participants
Current, if there's an image that Pillow can't process (e.g. an svg files), the plugin prints an error message but still changes the img src location. This means that the image in the final site won't load.
This change makes it to the img src is only changed if the image is processed without raising an error.
Ruff complains about the immediately re-raised error at about Line 760, but I've kept it to make it more obvious that the errors are expected to be raised at that location, and to make it clear that the function might be exited at that location. I've applied
#noqa: TRY203to that line to silence this particular warning.Ruff prints the following:
Edit: I've changed the log level for skipped images to
INFO(but left unfound images atWARNING). Partly, this is because I publish my site with fatal warnings, and so don't want it to break here.