[SVCS-407] Adding support for .psd files to MFR #269
Conversation
AddisonSchiller
changed the title
AddisonSchiller
changed the title
There was a problem hiding this comment.
The code runs as expected on my local machine 👍 .
PR looks great except one question on warnings and a few nick-picky style requests. Back to you @AddisonSchiller 🔥
| class BaseExporter(metaclass=abc.ABCMeta): | ||
|
|
||
| def __init__(self, source_file_path, output_file_path, format): | ||
| def __init__(self, ext, source_file_path, output_file_path, format): |
There was a problem hiding this comment.
For naming, normalized_name may be better then ext since it is consistent (LINK). Or consider full word extension if no conflicting/shadowing issue. I understand it is confusing since the normalized_name is indeed the extension name (LINK).
Our code base is not documented well. It will be great if we can gradually add doc string for functions and methods (e.g. for the ones that you modified, or the ones that you had a hard time to understand) that have many arguments. Here is an example where my description is not accurate and please revise:
def __init__(self, extension, source_file_path, output_file_path, format):
"""Initialize the base exporter.
:param extension: the name of the extension to be exported
:param source_file_path: the path of the input file
:param output_file_path: the path of the output file
:param format: the format of the exported file (e.g. 1200*1200.jpg)
"""
...There was a problem hiding this comment.
ext and self.ext was used as I saw it already being used elsewhere. Extension in MFR could refer to an actual extension library (eg the images extension, or Unoconv extension) So that is why it was avoided. normalized_name and ext are the same thing,
but the variable name doesnt make sense to me, so i did didn't use it.(dont remember why it is this way).
There was a problem hiding this comment.
Agreed. Let's keep using ext then and having the doc string should help for clarification.
| try: | ||
| image = Image.open(self.source_file_path) | ||
| if self.ext in ['.psd']: | ||
| # silence warnings from psd-tools |
There was a problem hiding this comment.
What are the warnings? Could this silence useful warnings as well?
There was a problem hiding this comment.
Warnings are custom thrown by the library. If there is an error, that still is caught. warnings look something like this:
/usr/local/lib/python3.5/site-packages/psd_tools/decoder/image_resources.py:138: UserWarning: ICC profile is not handled; colors could be incorrect. Please build PIL or Pillow with littlecms/littlecms2 support.
I remember doing some research into this when i originally did this PR, but Its been quite awhile and i can't remember much besides coming to the conclusion that it would be okay.
Warnings are silence so they dont spam the logs.
There was a problem hiding this comment.
I see. Warnings are fine since they are not exceptions. Your comment in the code also helps. We do want to catch exceptions thrown by psd-tools but maybe in another ticket so this is not a blocker.
| import os | ||
| import imghdr | ||
|
|
||
| import warnings |
There was a problem hiding this comment.
warnings is a standard python library (double check in case). Here is mine:
# standard python libarary
import os
import imghdr
import warnings
# third-party library
from PIL import Image
from psd_tools import PSDImage
# application code
from mfr.core import extension
from mfr.extensions.image import exceptions
|
|
Also added an ext variable to base_exporter. Extensions get swallowed in the image exporter. This serves as a work around to it.
AddisonSchiller
force-pushed
the
feature/psd-exporting
branch
from
9074724 to
cb0f92e
Compare
|
LGTMMTRTM |
3 participants
refs: https://openscience.atlassian.net/browse/SVCS-407
Purpose
add the .psd format to tabular file renderer
Summary of Changes
Added support to the image exporter to export .psd files as jpeg
Added the ext variable to the base exporter.
Testing Notes
There are 5-6 files on the jira ticket that can be used for testing. All should be renderable and not too large.