Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROB: Catch keyerror to allow duplicate fields #2333

Closed
wants to merge 1 commit into from

Conversation

pcraciunoiu
Copy link

Attempt to work around #2234

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3fb63f7) 94.43% compared to head (940601d) 94.40%.

Files Patch % Lines
pypdf/_writer.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2333      +/-   ##
==========================================
- Coverage   94.43%   94.40%   -0.03%     
==========================================
  Files          49       49              
  Lines        8008     8011       +3     
  Branches     1616     1616              
==========================================
+ Hits         7562     7563       +1     
- Misses        276      278       +2     
  Partials      170      170              

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

@MartinThoma MartinThoma changed the title Catch keyerror to allow duplicate fields ROB:Catch keyerror to allow duplicate fields Dec 9, 2023
@MartinThoma MartinThoma changed the title ROB:Catch keyerror to allow duplicate fields ROB: Catch keyerror to allow duplicate fields Dec 9, 2023
@MartinThoma
Copy link
Member

@stefan6419846 / @pubpub-zz What do you think about this PR?

@stefan6419846
Copy link
Collaborator

stefan6419846 commented Dec 10, 2023

Looking at the linked issues, I do not think that completely silencing these errors is the correct approach, while logger_warning does not seem to be correct either - such PDF files just are not supported by pypdf for now. I would certainly prefer a clean solution as pointed out in #2234 (comment).

@tagirahmad
Copy link

@stefan6419846 @pubpub-zz @pcraciunoiu Will this PR merged? I really need this functionality.

@stefan6419846
Copy link
Collaborator

stefan6419846 commented Jan 21, 2024

@tagirahmad I am no PDF expert, but as pointed out in #2333 (comment), the approach from this PR does not seem to be the correct/clean solution we are looking for IMHO.

@pcraciunoiu
Copy link
Author

I agree the solution is just to ignore an error that has a reason, but won't have time to look into the deeper fix anytime soon sadly.

@pubpub-zz
Copy link
Collaborator

fields could have multiple annotation:display at many places in a document but it is only one field which shall be unique.
allowing many fields with the same name is not a solution

pubpub-zz added a commit to pubpub-zz/pypdf that referenced this pull request Apr 2, 2024
stefan6419846 pushed a commit that referenced this pull request Apr 2, 2024
@stefan6419846
Copy link
Collaborator

Closing in favor of #2570.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants