algoo / preview-generator

generates previews of files with cache management
https://pypi.org/project/preview-generator/
MIT License
229 stars 51 forks source link

Untangle code checking if a program exist. Closes #98 #108

Closed JulienPalard closed 5 years ago

JulienPalard commented 5 years ago

What I try to fix

Avoid a call in executable_is_available

Also avoiding problems if the said process does not implement --version. Fixed by just using shutil.which.

While fixing this, I took the time to break the API (WTF ?), don't hesitate if it's not OK.

Undecided exception type

The implementations of check_dependencies were sometimes:

try:
    return check_executable_is_available("libreoffice")
except ExecutableNotFound:
    raise BuilderDependencyNotFound("this builder requires libreoffice to be available")

and sometimes:

return check_executable_is_available("inkscape")

Which were semantically equivalent: if it raises, something is wrong. But two distinct exceptions were semantically the same: BuilderDependencyNotFound and ExecutableNotFound. I tried to untangle this by removing ExecutableNotFound.

Unclear method protocol

check_executable_is_available and check_dependencies was said to return a boolean, but in fact never returned False. In one hand, returning a bool say "please use an if", but never returning False make the if useless.

Now check_executable_is_available is renamed as executable_is_available and just returns a boolean, never raises, as it's enough.

And check_dependencies now never returns True, just raises in case of a missing dependency.

The qpdf case

A missing qpdf were spotted only at runtime, only if given an PDF with an unspported code. I found this while refactoring for the previous cases. I don't like discovering missing dependencies at runtime, so I moved this check in a check_dependencies.

It make qpdf a dependency even when unnecessary, which is bad. But it make clear that qpdf can be necessary and has to be installed before a user try an incompatible PDF in production, which I think is a bit better.

bamthomas commented 5 years ago

Hello,

Would it be possible to merge and release this PR ?

I have unit tests for a small preview web server that fails because of the bad formatted logs :

logger.error("Error while checking dependencies: ", e)

And when I point to the github master in setup.py setuptools will still get the 0.11 version. Thank you.