Description
I agree with @alexaryn in that responsibility of closing a stream should rest purely on the caller of
PdfWriter
, and notPdfWriter
itself. Callingpdf_writer.close()
or using the context manager should only clean up the resources thatPdfWriter
itself has created as part of its operation. I apologize for not catching the current behavior when I reviewed #1193. 😬Looking at the code, there is also the inefficiency where
PdfWriter.__init__
is called twice. It's also permissible to callPdfWriter
with aPdfReader
as the first argument (to simplify cloning), but in doing so with contexts, will hit the following error:>>> with PdfReader("./sample-files/001-trivial/minimal-document.pdf") as reader: ... with PdfWriter(reader) as writer: ... print(writer.fileobj) ... __init__ __enter__ __init__ <pypdf._reader.PdfReader object at 0x10304e970> Traceback (most recent call last): File "<stdin>", line 4, in <module> File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 373, in __exit__ self.write(self.fileobj) File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 1396, in write self.write_stream(stream) File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 1367, in write_stream object_positions, free_objects = self._write_pdf_structure(stream) File "/Users/mpeveler/code/github/pypdf/pypdf/_writer.py", line 1500, in _write_pdf_structure stream.write(self.pdf_header.encode() + b"\n") AttributeError: 'PdfReader' object has no attribute 'write'
I'm not sure what the correct behavior here should be, especially with regards to cloning. Like, if I call:
with PdfWriter("foo.pdf") as writer:
and
foo.pdf
exists, do I expect that it'll be cloned into the writer? I'd think no? However, I think this does force the second__init__
call to be made as there's no way to tell in the first__init__
that we're in a context. I'd think this could be confusing for end users, but I guess not since no one has written in about this behavior.
Originally posted by @MasterOdin in #2905 (comment)
Activity
[-]errors when using context manager[/-][+]Cloning errors when using context manager[/+]BUG: Improve PdfWriter handing of context manager
alexaryn commentedon Oct 21, 2024
The API is ambiguous here, which makes a solution elusive. In a simple construction like
PdfWriter(f)
the semantics depend on iff
is an empty file or not. Empty means write tof
and non-empty means clone fromf
. This violates the principle of least astonishment. I would like it better if there were separate keyword arguments for "write to" and "clone from".The other main problem is that the context manager
__exit__()
is going to try to write to (and close) the file object, even if it was of the "clone from" kind and not writable.The double-init suggests that the logic and API can be tightened up.
stefan6419846 commentedon Oct 21, 2024
I have already requested changes on the corresponding PR, which is currently flawed and just introduced a magic positional argument which would guess its correct destination - while this would work if the flaws are removed, this just complicates the pypdf code unnecessarily as the user should know best what is desired in each case.
My proposal was to make the API more explicit by enforcing keyword-only arguments in the future (after a deprecation period). This way, we should at least avoid the ambiguity and force the user to set the correct value to avoid the current side effects.