Fix PDF preview (via print) #69
Conversation
janopae
force-pushed
the
fix_generating_temporary_PDFs_aka_print_preview
branch
from
6f76ce1
to
20e1289
Compare
| @@ -200,7 +200,7 @@ def __init__(self, doc: 'pml.Document'): | |||
| self.doc: pml.Document = doc | |||
|
|
|||
| # generate PDF document and return it as a string | |||
| def generate(self) -> str: | |||
| def generate(self) -> AnyStr: | |||
There was a problem hiding this comment.
| def generate(self) -> AnyStr: | |
| def generate(self) -> bytes: |
The str.encode method, which is used by the underlying call to util.toLatin1, returns bytes rather than AnyStr. There's a lot of history involved in the usage of str and bytes across Python 2 and 3, but unless this project needs to support Python 2, we should most likely be using str and bytes where appropriate rather than AnyStr (this is a bit of a generalized statement).
Is it a general goal of this project to adopt type annotations? If so, I'd be interested in filing an issue to use mypy for static type checking, since declared but unchecked types may lead to subtle bugs and confusion for new developers.
There was a problem hiding this comment.
Thanks!
I am a huge fan of static type analysis. If we had it, it would have prevented me from breaking the PDF preview in the first place. If mypy can be configured so that it checks the given types, but doesn't make problems for the code that doesn't have type declerations yet, I think it would be great if Trelby used it. @limburgher What do you think?
|
That sounds good. I use pylint on my personal laptop and in my internal BuildBot pipeline. |
When I changed the encoding of PDF files from UTF-8 to Latin1 in #60, I didn't realise that this changed the return type of PDFGenerator.generate(). This wasn't a problem for exporting PDFs, as the methods used there already could deal with
AnyStr(which means either the default UTF-8 encoded Python string orbytesencoded with another encoding), but the method for temporarily saving a PDF couldn't. This broke the PDF preview function.I already adjusted everything to support
AnyStrin #59, so I cherry-picked it from there.