Add save_pdf method #18
Conversation
tests/test_template.py
Outdated
| os.remove(path) | ||
| Path.rmdir(path.parent) | ||
| except FileNotFoundError: | ||
| with pytest.raises(LibreOfficeNotFoundError): |
There was a problem hiding this comment.
I would rather pytest.skip() if LibreOffice is not there.
Also, please limit the scope of this avialability test to the top call.
There was a problem hiding this comment.
What do you mean by this "limit the scope of this avialability test to the top call." ? Should I create special marker for this test method?
There was a problem hiding this comment.
Moving the except up immediately behind the availability check. Also comment on by @cmahr.
| """Saves the updated pptx to the specified filepath. | ||
| Args: | ||
| Args: |
There was a problem hiding this comment.
Please revert to reduce noise on git-diff :-)
|
|
||
| try: | ||
| subprocess.run(['libreoffice', '--version'], | ||
| stdout=subprocess.DEVNULL) # check if libreoffice is installed |
There was a problem hiding this comment.
Moving the except clause here would allow to reduce the indentation level in lines 110 to 125 (early return / guard clause). Maybe even extract in a private method à la _assert_libreoffice_available() to increase readability?
| stdout=subprocess.DEVNULL) # check if libreoffice is installed | ||
|
|
||
| path = Path(file_path) | ||
| outdir = path.parent |
There was a problem hiding this comment.
Maybe inline? Declaring/Initializing variables that are use 10 lines below tends to hinder my reading flow, because I instantaneously try to figure out why this variable is needed now...
| outdir = path.parent | ||
| file_name = path.name | ||
|
|
||
| # create temporary directory |
There was a problem hiding this comment.
After refactoring to tempfile.TemporaryDirectory() this comment seems superfluous.
| # TODO replace with self.save() method | ||
| self._presentation.save(template_temporary_path) | ||
|
|
||
| export_cmd = ['libreoffice', '--headless', '--convert-to', |
There was a problem hiding this comment.
I'm not a huge fan of defining these kind of commands in the middle of the other code, because this yet again hinders my reading flow. I would prefer to have a LibreOfficeWrapper class that encapsulates both checking that LibreOffice is present as well as converting a file to a PDF. This would also facilitate to write unit tests, because you could mock this class.
|
|
||
| export_cmd = ['libreoffice', '--headless', '--convert-to', | ||
| 'pdf', '--outdir', outdir, template_temporary_path] | ||
| p = subprocess.run( |
There was a problem hiding this comment.
It would be cool if we could do some error handling. Unfortunately, on my machine, the exit code is 0 regardless of the file being converted or not. Still, one way would be to capture the stderr and check for error messages:
stderr of a successful run:
javaldx: Could not find a Java Runtime Environment!
Warning: failed to read path from javaldx
stderr of an erroneous run:
javaldx: Could not find a Java Runtime Environment!
Warning: failed to read path from javaldx
Error: source file could not be loaded
There was a problem hiding this comment.
Since we don't know what to look for in stderr and it also contains output when being successful, investigating stderr is difficult.
Instead, I propose to check if the intended output file exists. If so, do something like
raise ConversionError(
f'Conversion to PDF using LibreOffice failed with error: {stderr}')
There was a problem hiding this comment.
Yes, checking whether the PDF file exists is a far simpler (and hence superior) idea. 👍
|
|
||
| _Pathlike = Union[str, pathlib.Path] | ||
|
|
||
| class LibreOfficeNotFoundError(FileNotFoundError): |
There was a problem hiding this comment.
Hmm... I don't really see the advantage of extending FileNotFoundError. Looking into the future, I would like to have a common base class for all pptx_blueprint-related exceptions, as this would allow for a 'catch all' except clause. And as far as I'm concerned, multiple inheritance is off the table ;-)
There was a problem hiding this comment.
Not sure I see the point of a 'catch all' specific for all pptx_blueprint errors. In particular, we're also not going to warp every possible exception that may be indirectly raised through our code (e.g. cannot save because disk is full).
However, it's ok to just derive from Exception here for the moment. We can later decide if we want to refine this.
There was a problem hiding this comment.
Having a parent exception does only make sense if we would catch all exceptions and rethrow library-specific exceptions, yes. It would allow the end user to write code like this:
try:
... do complicated stuff possibly throwing exceptions ...
pptx_blueprint.save_pdf()
... do other complicated stuff possibly throwing exceptions ...
except ComplicatedStuff1Exception as e:
... handle errors in complicated stuff no. 1 ...
except pptx_blueprint.ParentException as e:
... handle pdf creation errors ...
except Exception:
... handle other possible error paths ...This is not a must-have feature, but as an end user I like the convenience of it. Still, you'd have to catch and rethrow all exceptions, and I could perfectly understand anybody arguing that it's just not worth it :)
| try: | ||
| subprocess.run(['libreoffice', '--version'], | ||
| stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) # check if libreoffice is installed | ||
| output_path = 'test/test.pdf' |
There was a problem hiding this comment.
Could this be handled using tempfile.TemporaryDirectory(), too? This would make the test even more readable, because you could skip the tear down (lines 53 to 55).
There was a problem hiding this comment.
In pytest, this is even simplert with the tmp_path fixture.
http://doc.pytest.org/en/latest/tmpdir.html#the-tmp-path-fixture
| def test_save_pdf(template): | ||
| try: | ||
| subprocess.run(['libreoffice', '--version'], | ||
| stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) # check if libreoffice is installed |
There was a problem hiding this comment.
Similar to the comment in the Template::save_pdf() implementation, I would urge you to move the except clause here for an early return.
| template_temporary_path = f'{tmpdirname}/{file_name}' | ||
|
|
||
| # save current Template as pptx in temporary directory | ||
| # TODO replace with self.save() method |
There was a problem hiding this comment.
Please do so now, as the implementation of the save method was already merged into master.
4 participants
Implemented save_pdf method which works only for libreoffice on Linux OS.
Added one test method.