Corrections to image file handling:

[cybertoast]

• Handle cases where RGBA is not supported by PIL
• Set width and height of images to integer

[vstoykov]

@cybertoast thank you for your contribution!

About RGBA this is not the only place where we use RGBA. Probably we need to make some check/guessing if PIL is compiled with RGBA support or not and to prefer RGBA when available. Or if there are cases where RGBA is not needed even if available to not use it but to use the mod of original image.

About width and height that must be integer currently this is responsibility of the caller (instantiator) of the processor (for all processors). If we change that logic, we need to change it for all processors and not only for ResizeCanvas. Also the way that you are setting it is not good. If we want to change self.width, and self.height we need to make it in the __init__, instead of doing it in process. If we want to preserve original (float) value then rounding and make it integer need to be done in the process method but the values should not be set in self. This logic should be changed for all processors or there will be inconsistency which can confuse users.

Currently I'm kind of against these changes because:

1. Missing 'RGBA' means no PNGs which I doubt that someone will want in production. Also in my opinion this is undesirable consequence instead of desired one and PIL/Pillow just need to be recompiled with the needed libraries installed.
2. In all cases where we calculate width and height we convert it to int. In all other cases caller should know that PIL/Pillow works only with ints and converting it in pilkit everytime is kind of waste.

Still if you give me some arguments why this is needed I can reconsider my decision.

[cybertoast]

Thanks much for your response. Totally valid points and in thinking through my implementation further I agree that your are correct. I'll rescind my pull request.

On Dec 4, 2017 3:37 AM, "Venelin Stoykov" notifications@github.com wrote:

@cybertoast https://github.com/cybertoast thank you for your contribution!

About RGBA this is not the only place where we use RGBA. Probably we need to make some check/guessing if PIL is compiled with RGBA support or not and to prefer RGBA when available. Or if there are cases where RGBA is not needed even if available to not use it but to use the mod of original image.

About width and height that must be integer currently this is responsibility of the caller (instantiator) of the processor (for all processors). If we change that logic, we need to change it for all processors and not only for ResizeCanvas. Also the way that you are setting it is not good. If we want to change self.width, and self.height we need to make it in the init, instead of doing it in process. If we want to preserve original (float) value then rounding and make it integer need to be done in the process method but the values should not be set in self. This logic should be changed for all processors or there will be inconsistency which can confuse users.

Currently I'm kind of against these changes because:

1. Missing 'RGBA' means no PNGs which I doubt that someone will want in production. Also in my opinion this is undesirable consequence instead of desired one and PIL/Pillow just need to be recompiled with the needed libraries installed.
2. In all cases where we calculate width and height we convert it to int. In all other cases caller should know that PIL/Pillow works only with ints and converting it in pilkit everytime is kind of waste.

Still if you give me some arguments why this is needed I can reconsider my decision.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/matthewwithanm/pilkit/pull/29#issuecomment-348894521, or mute the thread https://github.com/notifications/unsubscribe-auth/AABBOmjLfma5Ofq8AGI0mgrrgbpuR9mMks5s869bgaJpZM4Qz8UB .