Add save_pdf method by fahro · Pull Request #18 · timhoffm/pptx-blueprint

fahro · 2019-10-13T13:21:04Z

Implemented save_pdf method which works only for libreoffice on Linux OS.
Added one test method.

pptx_blueprint/__init__.py

timhoffm · 2019-10-13T14:06:29Z

tests/test_template.py

+            os.remove(path)
+            Path.rmdir(path.parent)
+    except FileNotFoundError:
+        with pytest.raises(LibreOfficeNotFoundError):


I would rather pytest.skip() if LibreOffice is not there.

Also, please limit the scope of this avialability test to the top call.

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?

Moving the except up immediately behind the availability check. Also comment on by @cmahr.

tests/test_template.py

cmahr · 2019-10-16T18:51:32Z

pptx_blueprint/__init__.py

        """Saves the updated pptx to the specified filepath.

-        Args:
+         Args:


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

cmahr · 2019-10-16T18:54:31Z

pptx_blueprint/__init__.py

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


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?

cmahr · 2019-10-16T18:57:39Z

pptx_blueprint/__init__.py

+                           stdout=subprocess.DEVNULL)  # check if libreoffice is installed
+
+            path = Path(file_path)
+            outdir = path.parent


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

cmahr · 2019-10-16T18:58:53Z

pptx_blueprint/__init__.py

+            outdir = path.parent
+            file_name = path.name
+
+            # create temporary  directory


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

cmahr · 2019-10-16T19:04:25Z

pptx_blueprint/__init__.py

+                # TODO replace with self.save() method
+                self._presentation.save(template_temporary_path)
+
+                export_cmd = ['libreoffice', '--headless', '--convert-to',


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.

cmahr · 2019-10-16T19:13:59Z

pptx_blueprint/__init__.py

+
+                export_cmd = ['libreoffice', '--headless', '--convert-to',
+                              'pdf', '--outdir', outdir, template_temporary_path]
+                p = subprocess.run(


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

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

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

cmahr · 2019-10-16T19:17:32Z

pptx_blueprint/__init__.py


-_Pathlike = Union[str, pathlib.Path]
+
+class LibreOfficeNotFoundError(FileNotFoundError):


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

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.

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

cmahr · 2019-10-16T19:19:37Z

tests/test_template.py

+    try:
+        subprocess.run(['libreoffice', '--version'],
+                       stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)  # check if libreoffice is installed
+        output_path = 'test/test.pdf'


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

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

cmahr · 2019-10-16T19:21:02Z

tests/test_template.py

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


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

cmahr · 2019-10-16T19:27:57Z

pptx_blueprint/__init__.py

+                template_temporary_path = f'{tmpdirname}/{file_name}'
+
+                # save current Template as pptx in temporary directory
+                # TODO replace with self.save() method


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

fahro and others added 18 commits October 12, 2019 15:06

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

06b9f8d

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

452624d

implement find_shapes method.

03f5011

Merge branch 'master' into master

c2ac4c6

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

ba00b04

delete main.py

0d38757

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

55b1864

merge

064ea73

Refactoring

aa02d9c

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

3527b81

Merge branch 'master' of github.com:fahro/pptx-blueprint

61e240c

merge

b51385f

merge

2467f4d

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

03717bc

refactor code PEP8

8499b97

Merge branch 'master' of https://github.com/timhoffm/pptx-blueprint

9061a6a

add save_pdf method with test

2a41e13

fix test for save_pdf

e035e4c

fahro mentioned this pull request Oct 13, 2019

Export to PDF #12

Open

timhoffm reviewed Oct 13, 2019

View reviewed changes

fahro added 2 commits October 13, 2019 22:12

refactor save_pdf method

79a3bc2

small fix save_pdf

b408a9e

cmahr approved these changes Oct 16, 2019

View reviewed changes

cmahr reviewed Oct 16, 2019

View reviewed changes


		_Pathlike = Union[str, pathlib.Path]

		class LibreOfficeNotFoundError(FileNotFoundError):

Comments

Conversation

fahro commented Oct 13, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmahr Oct 16, 2019 • edited by timhoffm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timhoffm Oct 16, 2019 •

edited

Loading

cmahr Oct 16, 2019 •

edited by timhoffm

Loading

timhoffm Oct 16, 2019 •

edited

Loading