Skip to content

ROB: Catch keyerror to allow duplicate fields#2333

Closed
pcraciunoiu wants to merge 1 commit intopy-pdf:mainfrom
uplift-ltd:catch-keyerror
Closed

ROB: Catch keyerror to allow duplicate fields#2333
pcraciunoiu wants to merge 1 commit intopy-pdf:mainfrom
uplift-ltd:catch-keyerror

Conversation

@pcraciunoiu
Copy link
Copy Markdown

@pcraciunoiu pcraciunoiu commented Dec 8, 2023

Attempt to work around #2234

@pcraciunoiu pcraciunoiu mentioned this pull request Dec 8, 2023
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 8, 2023 •
edited
Loading

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
Copy Markdown
Member

MartinThoma commented Dec 10, 2023

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

@stefan6419846
Copy link
Copy Markdown
Collaborator

stefan6419846 commented Dec 10, 2023 •
edited
Loading

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
Copy Markdown

tagirahmad commented Jan 21, 2024

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

@stefan6419846
Copy link
Copy Markdown
Collaborator

stefan6419846 commented Jan 21, 2024 •
edited
Loading

@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
Copy Markdown
Author

pcraciunoiu commented Jan 21, 2024

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
Copy Markdown
Collaborator

pubpub-zz commented Mar 30, 2024

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
@pubpub-zz
BUG: fix fields update where annots are kids of field
d7b286e 
closes py-pdf#2234
closes py-pdf#2512

replaces  py-pdf#2333
@pubpub-zz pubpub-zz mentioned this pull request Apr 2, 2024
Merged
stefan6419846 pushed a commit that referenced this pull request Apr 2, 2024
@pubpub-zz
BUG: Fix fields update where annotations are kids of field (#2570)
50bb7f2 
Closes #2234.
Closes #2512.

Replaces  #2333.
@stefan6419846
Copy link
Copy Markdown
Collaborator

stefan6419846 commented Apr 2, 2024

Closing in favor of #2570.

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

Reviewers

@pubpub-zz pubpub-zz Awaiting requested review from pubpub-zz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pcraciunoiu @MartinThoma @stefan6419846 @tagirahmad @pubpub-zz