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

42
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 todoc(self): def finalize(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,11 +1267,9 @@ class pdfdoc(object):
self.writer.addobj(metadata) self.writer.addobj(metadata)
self.writer.addobj(iccstream) self.writer.addobj(iccstream)
return self.writer, self.output_version
def tostream(self, outputstream): def tostream(self, outputstream):
# write out the PDF # write out the PDF
self.todoc() # finalize self.writer # this assumes that finalize() has been invoked beforehand by the caller
if self.engine == Engine.pikepdf: if self.engine == Engine.pikepdf:
kwargs = {} kwargs = {}
if pikepdf.__version__ >= "6.2.0": if pikepdf.__version__ >= "6.2.0":
@ -2610,14 +2608,11 @@ def find_scale(pagewidth, pageheight):
return 10 ** ceil(log10(oversized)) return 10 ** ceil(log10(oversized))
# given one or more input image, depending on outputstream, either return a # Convert the image(s) to a `pdfdoc` object.
# string containing the whole PDF if outputstream is None or write the PDF # The `.writer` attribute holds the underlying engine document handle, and
# data to the given file-like object and return None # `.output_version` the minimum version the caller should use when saving.
# # The main convert() wraps this implementation function.
# Input images can be given as file like objects (they must implement read()), def convert_to_docobject(*images, **kwargs):
# as a binary string representing the image content or as filenames to the
# images.
def convert(*images, **kwargs):
_default_kwargs = dict( _default_kwargs = dict(
engine=None, engine=None,
title=None, title=None,
@ -2638,8 +2633,6 @@ def convert(*images, **kwargs):
viewer_fit_window=False, viewer_fit_window=False,
viewer_center_window=False, viewer_center_window=False,
viewer_fullscreen=False, viewer_fullscreen=False,
outputstream=None,
return_engine_doc=False,
first_frame_only=False, first_frame_only=False,
allow_oversized=True, allow_oversized=True,
cropborder=None, cropborder=None,
@ -2802,13 +2795,22 @@ def convert(*images, **kwargs):
iccp, iccp,
) )
if kwargs["outputstream"]: pdf.finalize()
pdf.tostream(kwargs["outputstream"]) return pdf
# given one or more input image, depending on outputstream, either return a
# string containing the whole PDF if outputstream is None or write the PDF
# data to the given file-like object and return None
#
# Input images can be given as file like objects (they must implement read()),
# as a binary string representing the image content or as filenames to the
# images.
def convert(*images, outputstream=None, **kwargs):
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.
pdf = convert_to_docobject(*images, **kwargs)
if outputstream:
pdf.tostream(outputstream)
return return
if kwargs["return_engine_doc"]:
return pdf.todoc()
return pdf.tostring() return pdf.tostring()

5
src/img2pdf_test.py
View file

@ -7149,9 +7149,10 @@ def test_general(general_input, engine):
def test_return_engine_doc(tmp_path_factory): def test_return_engine_doc(tmp_path_factory):
inputf = os.path.join(os.path.dirname(__file__), "tests", "input", "normal.jpg") inputf = os.path.join(os.path.dirname(__file__), "tests", "input", "normal.jpg")
outputf = tmp_path_factory.mktemp("return_engine_doc") / "normal.jpg.pdf" outputf = tmp_path_factory.mktemp("return_engine_doc") / "normal.jpg.pdf"
pdf, min_version = img2pdf.convert(inputf, return_engine_doc=True, engine=img2pdf.Engine.pikepdf) pdf_wrapper = img2pdf.convert_to_docobject(inputf, engine=img2pdf.Engine.pikepdf)
pdf = pdf.writer
assert isinstance(pdf, pikepdf.Pdf) assert isinstance(pdf, pikepdf.Pdf)
pdf.save(outputf, min_version=min_version, linearize=True) pdf.save(outputf, min_version=pdf_wrapper.output_version, linearize=True)
assert os.path.isfile(outputf) assert os.path.isfile(outputf)