Skip to content

Add save_pdf method #18

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.

Sign up for GitHub

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Add save_pdf method #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
06b9f8d
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 12, 2019
452624d
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 12, 2019
03f5011
implement find_shapes method.
fahro Oct 12, 2019
c2ac4c6
Merge branch 'master' into master
lysnikolaou Oct 12, 2019
ba00b04
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 12, 2019
0d38757
delete main.py
fahro Oct 12, 2019
55b1864
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 12, 2019
064ea73
merge
fahro Oct 12, 2019
aa02d9c
Refactoring
lysnikolaou Oct 12, 2019
3527b81
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 12, 2019
61e240c
Merge branch 'master' of github.com:fahro/pptx-blueprint
fahro Oct 12, 2019
b51385f
merge
fahro Oct 12, 2019
2467f4d
merge
fahro Oct 12, 2019
03717bc
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 12, 2019
8499b97
refactor code PEP8
fahro Oct 13, 2019
9061a6a
Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint
fahro Oct 13, 2019
2a41e13
add save_pdf method with test
fahro Oct 13, 2019
e035e4c
fix test for save_pdf
fahro Oct 13, 2019
79a3bc2
refactor save_pdf method
fahro Oct 13, 2019
b408a9e
small fix save_pdf
fahro Oct 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions pptx_blueprint/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import pathlib
from pathlib import Path
import pptx
from typing import Union, Iterable, Tuple
from pptx.shapes.base import BaseShape
import subprocess
import tempfile
_Pathlike = Union[str, Path]

_Pathlike = Union[str, pathlib.Path]

class LibreOfficeNotFoundError(FileNotFoundError):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

pass


class Template:
Expand Down Expand Up @@ -85,8 +90,38 @@ def _find_shapes_in_slide(slide):
def save(self, filename: _Pathlike) -> None:
"""Saves the updated pptx to the specified filepath.

Args:
Args:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert to reduce noise on git-diff :-)

filename (path-like): file name or path
"""
# TODO: make sure that the user does not override the self._template_path
pass

def save_pdf(self, file_path: _Pathlike) -> None:
"""Exports the updated pptx to the specified filepath as pdf file.

Args:
file_path (path-like) file name or path
"""

try:
subprocess.run(['libreoffice', '--version'],
stdout=subprocess.DEVNULL) # check if libreoffice is installed
Copy link

@cmahr cmahr Oct 16, 2019 •
edited by timhoffm
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


path = Path(file_path)
outdir = path.parent
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

file_name = path.name

# create temporary directory
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After refactoring to tempfile.TemporaryDirectory() this comment seems superfluous.

with tempfile.TemporaryDirectory() as tmpdirname:
template_temporary_path = f'{tmpdirname}/{file_name}'

# save current Template as pptx in temporary directory
# TODO replace with self.save() method
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do so now, as the implementation of the save method was already merged into master.

self._presentation.save(template_temporary_path)

export_cmd = ['libreoffice', '--headless', '--convert-to',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'pdf', '--outdir', outdir, template_temporary_path]
p = subprocess.run(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

@timhoffm timhoffm Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}')

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, checking whether the PDF file exists is a far simpler (and hence superior) idea. 👍

export_cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
except FileNotFoundError:
raise LibreOfficeNotFoundError("Libre Office not found.")
19 changes: 18 additions & 1 deletion tests/test_template.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# TODO
from pptx_blueprint import Template
from pptx_blueprint import Template, LibreOfficeNotFoundError
from pathlib import Path
import pytest
import os
import subprocess


@pytest.fixture
Expand Down Expand Up @@ -38,3 +40,18 @@ def test_find_shapes_from_one_slide(template):
def test_find_shapes_index_out_of_range(template):
with pytest.raises(IndexError):
shapes = template._find_shapes(0, 'logo')


def test_save_pdf(template):
try:
subprocess.run(['libreoffice', '--version'],
stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) # check if libreoffice is installed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the comment in the Template::save_pdf() implementation, I would urge you to move the except clause here for an early return.

output_path = 'test/test.pdf'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In pytest, this is even simplert with the tmp_path fixture.
http://doc.pytest.org/en/latest/tmpdir.html#the-tmp-path-fixture

template.save_pdf(output_path)
path = Path(output_path)
assert path.exists() == True
if path.exists():
os.remove(path)
Path.rmdir(path.parent)
except FileNotFoundError:
pytest.skip()