STY: Type the destination and field trees as DictionaryObject - #3988
Merged
stefan6419846 merged 3 commits intoAug 24, 2026
Merged
Conversation
_get_named_destinations and _get_fields both walk name trees that are plain dictionaries: a name tree node holds /Kids or /Names, and the AcroForm is a dictionary too. Neither is a TreeObject, and neither function calls anything TreeObject adds - both only do dictionary access. The casts asserted TreeObject on objects that are DictionaryObject at runtime, so a typeguard run reports 16 failures across the reader, writer and form tests. Typed to match what is passed, following _check_kids in the same file, which already accepts either.
PdfReader overrides _get_named_destinations, so narrowing it to TreeObject while the base class accepts a DictionaryObject as well breaks substitution.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3988 +/- ##
==========================================
+ Coverage 97.95% 97.98% +0.02%
==========================================
Files 57 57
Lines 11058 11098 +40
Branches 2072 2078 +6
==========================================
+ Hits 10832 10874 +42
+ Misses 126 125 -1
+ Partials 100 99 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stefan6419846
requested changes
Aug 21, 2026
TreeObject subclasses DictionaryObject, so naming both adds nothing.
Contributor
Author
|
No benefit, you're right — TreeObject subclasses DictionaryObject, so naming both adds nothing. Dropped it, and the signature is now Optional[DictionaryObject] in both _get_named_destinations and _get_fields, plus the PdfReader override so it still matches. _check_kids further down the same file has the same redundant union, but I left it alone since it's outside what this PR touches. Happy to clean it up here or separately if you'd like. |
stefan6419846
approved these changes
Aug 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_get_named_destinationsand_get_fieldsboth walk trees that are plain dictionaries at runtime. A name tree node holds/Kidsor/Names, and the AcroForm is a dictionary too — neither is aTreeObject, and neither function calls anythingTreeObjectadds on top ofDictionaryObject. Both only do dictionary access.The casts asserted
TreeObjecton objects that areDictionaryObject, so a typeguard run reports 16 failures across the reader, writer and form tests._check_kidsin the same file already takesUnion[TreeObject, DictionaryObject], so I followed that.Reverting the change brings the 16 failures back.