MAINT: Increase readability of _merge_page

[j-t-1]

No description provided.

[j-t-1]

This PR is wrong, I should not have moved the annotations. Will either close or change...

[stefan6419846]

I am going to mark this PR as draft. Feel free to either close or mark it ready for review if you decided on an approach.

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.99%. Comparing base (b622a2f) to head (c0384d5).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
pypdf/_page.py	95.83%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3291      +/-   ##
==========================================
+ Coverage   96.97%   96.99%   +0.01%     
==========================================
  Files          54       56       +2     
  Lines        9324     9384      +60     
  Branches     1708     1714       +6     
==========================================
+ Hits         9042     9102      +60     
  Misses        168      168              
  Partials      114      114              

☔ 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.

[j-t-1]

@stefan6419846 code coverage has decreased but seems as if it should be the same? Moving the annotations is okay also.

[stefan6419846]

The case where https://github.com/py-pdf/pypdf/pull/3291/files#diff-153e5cee71ddf471246ff5c0e198345f005942a540a8f39297736f0b620d631dR1225 does not hold true is not covered by the tests (although it has never been as far as I can see). Are we able to test this as well?

[j-t-1]

Which test needs changing?

[stefan6419846]

See coverage/my previous comment.

[j-t-1]

if isinstance(annots, ArrayObject): needs to be covered. Is there a way to go from the coverage of the lines above or below this line to the tests they are covered in?

[stefan6419846]

I am not aware of this unless you modify the coverage configuration. In my cases, I usually run the tests with raising an exception in this specific section locally.

[j-t-1]

Unsure why this is giving an error.

Is there a PDF in the resources folder with multiple annotations on a page?

[stefan6419846]

Why are you trying to merge a page with itself? This looks somehow wrong.

[j-t-1]

I copied the idea, see test_no_resources, but that uses PdfReader. But agree, probably not best practice.

[stefan6419846]

Unsure why this is giving an error.

This seems to be a bug which already existed previously, although I am not sure why this never has been a problem in the past. The underlying problem is that we try to create a new Polygon instance, but the default code from DictionaryObject.clone expects that the constructors of the objects to clone does not require parameters, which seems to fail for all markup annotations. We would have to provide a generic way to define a mapping of own PDF keys to parameters to allow for such use cases.