Skip to content

PPF-2075: Reject ignored arguments and distinguish empty input from invalid input - #2092

Merged
chinapandaman merged 3 commits into
masterfrom
PPF-2075
Oct 30, 2026
Merged

chinapandaman merged 3 commits into
masterfrom
PPF-2075

Conversation

@chinapandaman

@chinapandaman chinapandaman commented Oct 30, 2026 •

Copy link
Copy Markdown
Owner

Resolves #2075 (action plan item E2-05, review finding R10).

Changes

  • Keyword arguments: PdfWrapper(...) and fill() raise TypeError for removed or unknown keywords before any mutation. preserve_metadata and flatten report the release that removed them and their replacement (REMOVED_KWARGS); other keywords list the supported ones.
  • File inputs: the shared adapter opens str and PathLike paths directly, so missing paths raise FileNotFoundError for templates, fonts, signature/image values, and RawImage. Unsupported types raise TypeError. PdfWrapper(), b"", and empty streams still create an empty wrapper.
  • PdfArray.merge(): an empty array returns a new empty PdfWrapper, and the result is never one of the inputs. One-item and empty-padded arrays return an independent copy (PdfWrapper._copy()) that reuses the current stream and cached PDF version without embedding registered fonts again, and deep-copies widget settings and pending key updates. + and slicing identity are unchanged (E4-02).
  • Structure: PdfArray moved from lib/types.py into lib/wrapper.py to remove the circular import; from PyPDFForm import PdfArray is unchanged. Pylint C0302 (module line limit) is disabled.
  • Tests: test_dropdown_tutorial registered a nonexistent font that was silently skipped; the dead call is removed and PPF-1599_expected.pdf stays byte-identical. test_register_bad_fonts now expects FileNotFoundError for a missing font path.
  • Docs: migration guide section on silently ignored arguments (6.0.0–6.0.2 only), adapter tip in basic.md, bulk-merge note in utils.md, docstrings, and the E2-05 action plan entry.

Validation

make test-all (456 passed, 100% coverage), make linting, strict MkDocs build, and git diff --check, using local ./venv with standard zlib. The development container, the cross-platform matrix, and viewer rendering were not run locally. No appearance generation changed.

🤖 Generated with Claude Code

- Raise TypeError for removed or unknown PdfWrapper/fill keyword arguments,
  with per-keyword removal version and migration guidance.
- Make the file adapter open str/PathLike paths directly and reject
  unsupported types instead of returning empty input.
- Make PdfArray.merge() return a new wrapper for empty and one-item arrays;
  move PdfArray into wrapper.py to remove the circular import.
- Update docs, migration guide, and the E2-05 action plan entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chinapandaman chinapandaman self-assigned this Oct 30, 2026
@chinapandaman chinapandaman added the enhancement New feature or request label Oct 30, 2026
@chinapandaman chinapandaman moved this from Backlog to In review in PyPDFForm-support-2026 Oct 30, 2026
@chinapandaman

Copy link
Copy Markdown
Owner Author

Reviewed commit 83b713c841de82b86e3924de2afb2e73d897ae03. I found two issues in the new merge-copy path:

  1. [P2] Preserve live widget settings when copying a wrapper — wrapper.py, line 1085.

    Reconstructing the wrapper from _read() loses settings that exist only on the middleware objects. For example:

    source = PdfWrapper("pdf_samples/signature/sample_template_with_signature_overlap.pdf")
    source.widgets["signature"].preserve_aspect_ratio = False
    result = PdfArray([source, PdfWrapper()]).merge()
    assert result.widgets["signature"].preserve_aspect_ratio is False  # fails: True

    Filling the result with image_samples/small_icon.png consequently draws the image at different dimensions. The base commit preserves this setting for the same empty-padded merge. Please preserve an independent copy of the live widget state alongside the PDF bytes and add regression coverage.

  2. [P2] Avoid embedding registered fonts again when copying — wrapper.py, lines 245–246.

    Unlike a merge that rebuilds resources, this copy starts with bytes that already contain the registered fonts. Replaying register_font() embeds each font again. With sample_template.pdf and LiberationSerif-BoldItalic.ttf registered as custom, a one-item merge grows the PDF from 715,614 to 806,775 bytes and adds /F2 alongside /F1. Each subsequent copy adds another 91,161 bytes without changing the document content. Please preserve the existing resources and copy the font mappings and registration bookkeeping without embedding again.

Validation: make check-zlib reported standard zlib; make test-all passed all 451 tests with 100% coverage; make linting, a strict MkDocs build, and git diff --check passed. Both findings were reproduced separately; the current suite does not catch them.

- Replace the rebuild-and-replay copy path in PdfArray.merge() with
  PdfWrapper._copy(), which reuses the current stream without embedding
  registered fonts again and deep-copies widgets and pending key updates.
- Restore PdfWrapper.__add__ to its original implementation.
- Add regression coverage and record the review follow-up in the plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chinapandaman

Copy link
Copy Markdown
Owner Author

Re-reviewed commit 2344371cbf86304eef58012c03917721eb767859. Both earlier findings are fixed: signature settings survive copying, and repeated copies preserve the PDF bytes and embedded font resources without growing the file.

One remaining issue:

[P2] Preserve the cached PDF version when copying a wrapper — wrapper.py, lines 251–258.

source = PdfWrapper("pdf_samples/sample_template.pdf").change_version("2.0")
source.widgets["test"].font_size = 18
result = PdfArray([source, PdfWrapper()]).merge()

assert result.version == source.version  # fails: "1.3" != "2.0"
assert result.read().startswith(b"%PDF-2.0")  # also fails

_copy() calls _read(), which applies the pending style hook through a writer that emits PDF 1.3. The source retains its cached _version and restores the 2.0 header when serialized, but the copy initializes _version to None and subsequently treats the rewritten 1.3 header as authoritative. The same issue reproduces with 1.7. The base commit preserves the version for this empty-padded merge.

Please copy the cached version alongside the other wrapper state and add regression coverage for a pending style change followed by a one-item or empty-padded merge, asserting both the version property and serialized header.

Validation: standard zlib; the two updated merge tests passed; make test-all passed all 452 tests with 100% coverage; make linting and git diff --check passed. The version issue was reproduced separately and compared with the base behavior.

- Copy the cached PDF version in PdfWrapper._copy() so copies restore the
  source header after hook rewrites emit a different version.
- Add 1.7/2.0 regression coverage for one-item and empty-padded merges after
  a pending style change, and record the follow-up in the plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chinapandaman
chinapandaman merged commit 80d4538 into master Oct 30, 2026
20 checks passed
@chinapandaman
chinapandaman deleted the PPF-2075 branch October 30, 2026 03:51
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPDFForm-support-2026 Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

PPF-2075: Reject ignored arguments and distinguish empty input from invalid input

1 participant