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 +8 -8
1 changed files with 8 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit c1c77a7453 - Show all commits

14
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=None, return_engine_doc=False): def todoc(self):
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
@ -1267,12 +1267,11 @@ class pdfdoc(object):
self.writer.addobj(metadata) self.writer.addobj(metadata)
self.writer.addobj(iccstream) self.writer.addobj(iccstream)
if return_engine_doc:
return self.writer, self.output_version return self.writer, self.output_version
# now write out the PDF def tostream(self, outputstream):
if outputstream is None: # write out the PDF
raise TypeError("pdfdoc.tostream() requires outputstream unless return_engine_doc is True.") self.todoc() # finalize self.writer
if self.engine == Engine.pikepdf: if self.engine == Engine.pikepdf:
kwargs = {} kwargs = {}
if pikepdf.__version__ >= "6.2.0": if pikepdf.__version__ >= "6.2.0":
@ -1281,6 +1280,7 @@ class pdfdoc(object):
outputstream, min_version=self.output_version, linearize=True, **kwargs outputstream, min_version=self.output_version, linearize=True, **kwargs
) )
elif self.engine == Engine.pdfrw: elif self.engine == Engine.pdfrw:
from pdfrw import PdfName, PdfArray
self.writer.trailer.Info = self.writer.docinfo self.writer.trailer.Info = self.writer.docinfo
# setting the version attribute of the pdfrw PdfWriter object will # setting the version attribute of the pdfrw PdfWriter object will
# influence the behaviour of the write() function # influence the behaviour of the write() function
@ -2803,11 +2803,11 @@ def convert(*images, **kwargs):
) )
if kwargs["outputstream"]: if kwargs["outputstream"]:
pdf.tostream(outputstream=kwargs["outputstream"]) pdf.tostream(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(return_engine_doc=True) return pdf.todoc()
return pdf.tostring() return pdf.tostring()