josch/img2pdf
3
6
Fork 10
Code Issues 22 Pull requests 5 Projects Releases 3 Wiki Activity

convert(): add option to get engine document (e.g. pikepdf.Pdf) #203

Open
mara0004 wants to merge 7 commits from mara0004/img2pdf:return_engine_doc into main
pull from: mara0004/img2pdf:return_engine_doc
merge into: josch:main
Conversation 7 Commits 7 Files changed 2 +5 -3
1 changed files with 5 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit d4b49c510a - Show all commits

8
src/img2pdf.py
View file

@ -1075,7 +1075,7 @@ class pdfdoc(object):
self.tostream(stream) self.tostream(stream)
return stream.getvalue() return stream.getvalue()
def tostream(self, outputstream, return_engine_doc=False): def tostream(self, outputstream=None, return_engine_doc=False):
josch commented 2024-09-15 06:01:12 +00:00
Review
Copy link

You split tostream() into finalize() and tostream() but then why does the new tostream() not call finalize()?

You split `tostream()` into `finalize()` and `tostream()` but then why does the new `tostream()` not call `finalize()`?
mara0004 commented 2024-09-15 14:14:13 +00:00
Review
Copy link

Because I though the embedder of convert_to_docobject() should not have to invoke finalize().
Instead, finalize() technically belongs into convert_to_docobject() itself, after all image pages have been added. So tostream() cannot also finalize() as that would result in a double call.

Because I though the embedder of `convert_to_docobject()` should not have to invoke `finalize()`. Instead, `finalize()` technically belongs into `convert_to_docobject()` itself, after all image pages have been added. So `tostream()` cannot also `finalize()` as that would result in a double call.
if self.engine == Engine.pikepdf: if self.engine == Engine.pikepdf:
PdfArray = pikepdf.Array PdfArray = pikepdf.Array
PdfDict = pikepdf.Dictionary PdfDict = pikepdf.Dictionary
@ -1271,6 +1271,8 @@ class pdfdoc(object):
return self.writer, self.output_version return self.writer, self.output_version
# now write out the PDF # now write out the PDF
if outputstream is None:
raise TypeError("pdfdoc.tostream() requires outputstream unless return_engine_doc is True.")
if self.engine == Engine.pikepdf: if self.engine == Engine.pikepdf:
kwargs = {} kwargs = {}
if pikepdf.__version__ >= "6.2.0": if pikepdf.__version__ >= "6.2.0":
@ -2801,11 +2803,11 @@ def convert(*images, **kwargs):
) )
if kwargs["outputstream"]: if kwargs["outputstream"]:
pdf.tostream(kwargs["outputstream"]) pdf.tostream(outputstream=kwargs["outputstream"])
return return
if kwargs["return_engine_doc"]: if kwargs["return_engine_doc"]:
josch commented 2024-09-15 05:59:41 +00:00
Review
Copy link

Please do not change the signature of the convert() function. This is necessary to preserve API stability. You have to extract "outputstream" from kwargs.

Please do not change the signature of the convert() function. This is necessary to preserve API stability. You have to extract "outputstream" from kwargs.
mara0004 commented 2024-09-15 15:29:38 +00:00
Review
Copy link

It's not clear to me why this is supposed to break the API -- can you explain?
AFAICS, kwargs is internal, and no callee expects kwargs["outputstream"], right?

It's not clear to me why this is supposed to break the API -- can you explain? AFAICS, `kwargs` is internal, and no callee expects `kwargs["outputstream"]`, right?
mara0004 commented 2024-09-15 15:40:35 +00:00
Review
Copy link

Relatedly, it looks like the _default_kwargs strategy silently ignores nonexistent parameters, which is problematic. (And of course it breaks IDE completion.)
I see you're using kwargs to unify access of {crop,bleed,trim,art}border, which is fine, but why can't any others params be in the signature directly?

Relatedly, it looks like the `_default_kwargs` strategy silently ignores nonexistent parameters, which is problematic. (And of course it breaks IDE completion.) ~~I see you're using kwargs to unify access of `{crop,bleed,trim,art}border`, which is fine, but why can't any others params be in the signature directly?~~
mara0004 commented 2024-09-16 21:21:24 +00:00
Review
Copy link

It's not clear to me why this is supposed to break the API -- can you explain?

Okay, self-answering the question: a SO search yielded that specifying optional params after a *capture raises a SyntaxError on Python 2. Never having written code for Python 2, I did not know that. I'll change to extract from kwargs as you said, then.

The issue with invalid kwargs assumably being ignored without error still stands, though.

> It's not clear to me why this is supposed to break the API -- can you explain? Okay, self-answering the question: a SO search yielded that specifying optional params after a `*capture` raises a `SyntaxError` on Python 2. Never having written code for Python 2, I did not know that. I'll change to extract from kwargs as you said, then. The issue with invalid kwargs assumably being ignored without error still stands, though.
return pdf.tostream(None, return_engine_doc=True) return pdf.tostream(return_engine_doc=True)
return pdf.tostring() return pdf.tostring()