Skip to content

ROB: ignore annotation with Go-To action missing a /D name attribute (#3211)#3213

Closed
mlorant wants to merge 1 commit intopy-pdf:mainfrom
mlorant:main
Closed

ROB: ignore annotation with Go-To action missing a /D name attribute (#3211)#3213
mlorant wants to merge 1 commit intopy-pdf:mainfrom
mlorant:main

Conversation

@mlorant
Copy link

@mlorant mlorant commented Mar 22, 2025

This PR fixes a bug encountered in issue #3211.

I've being looking through the official Acrobat PDF standard specifications, and this /D field should be present for Go-To actions, however the code already has a case where this /D is a NullObject, so I guess it is fine to consider the non-existence case similarly.

@codecov
Copy link

codecov bot commented Mar 22, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.51%. Comparing base (fda2c9b) to head (3ce71d0).
⚠️ Report is 113 commits behind head on main.

Files with missing lines Patch % Lines
pypdf/_writer.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3213      +/-   ##
==========================================
- Coverage   96.53%   96.51%   -0.03%     
==========================================
  Files          53       53              
  Lines        8930     8933       +3     
  Branches     1642     1643       +1     
==========================================
+ Hits         8621     8622       +1     
- Misses        186      187       +1     
- Partials      123      124       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mlorant
Copy link
Author

mlorant commented Mar 22, 2025

Sorry for the mess with the force-push, I'm not being used to mypy, my initial patch was not passing the code style check 😅

@stefan6419846
Copy link
Collaborator

Thanks for the PR. As mentioned in the issue, this still needs a test to ensure that this will not break again in the future.

By the way: No need to force push anything here - commits are always squashed on merge.

@stefan6419846 stefan6419846 added the needs-test A test should be added before this PR is merged. label Mar 22, 2025
stefan6419846 added a commit that referenced this pull request May 8, 2025
@stefan6419846
Copy link
Collaborator

Fixed properly in #3281.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-test A test should be added before this PR is merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants