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

Fix written image-format #42

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

Conversation

olaf-mandel
Copy link
Contributor

The default behaviour is to name all files as <basename>.jpg on the assumption that all images are downloaded with type image/jpeg. But this is not necessarily true, images may also be downloaded as image/png in which case the file-name is wrong.

To fix this, add a new choice native to the option --images-format and have it detect the actual downloaded image type from the Content-Type header. Make this new choice the (changed) default choice. Keep the choices jpg and png but enforce conversion in both cases if the downloaded image does not match the desired format.

The default behaviour is to name all files as `<basename>.jpg` on the
assumption that all images are downloaded with type `image/jpeg`. But
this is not necessarily true, images may also be downloaded as
`image/png` in which case the file-name is wrong.

To fix this, add a new choice `native` to the option `--images-format`
and have it detect the actual downloaded image type from the
`Content-Type` header. Make this new choice the (changed) default
choice. Keep the choices `jpg` and `png` but enforce conversion in both
cases if the downloaded image does not match the desired format.
@Zehina
Copy link
Owner

Zehina commented Dec 31, 2023

Thank you again @olaf-mandel for all the improvements you've submitted. I was looking at merging either before or after #36 since that changes the whole structure of the project.

This looks good to me so just to tackle how this is going to interact with #36 as that branch for now forces a single image-format to be used but does save the files corectly. My guess is that adding native as an option will just not add any AioImageFormatTransformer to the list of transformers.

  • If you don't mind you can port your changes to a branch that targets new- structure which I think can relatively be easily done there as well. But using a different way.
  • Or we can merge this for now and I'll work on porting it to new-structure.

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