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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.59%. Comparing base (827bf7f) to head (46cd1ca).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3291   +/-   ##
=======================================
  Coverage   97.59%   97.59%           
=======================================
  Files          55       55           
  Lines       10228    10228           
  Branches     1877     1877           
=======================================
  Hits         9982     9982           
  Misses        141      141           
  Partials      105      105           

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

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.

[stefan6419846]

It seems like the old comments magically appeared yesterday. I have commented on the specific review comments.

[j-t-1]

@stefan6419846 I think there is something wrong with this outside of this PR.

I raised an issue #3747, and you closed it as already an issue and PR. That PR has a test that would effectively duplicate the test here (their test is actually better).

[stefan6419846]

That PR has a test that would effectively duplicate the test here (their test is actually better).

Just to recap as there are tons of comments on this PR: Which case needed to be covered by tests? Is it really necessary to include the faulty branch for this?

[j-t-1]

That PR has a test that would effectively duplicate the test here (their test is actually better).

Just to recap as there are tons of comments on this PR: Which case needed to be covered by tests? Is it really necessary to include the faulty branch for this?

This PR will pass if it is just one annotation added (in either direction). The more natural test is adding an annotated page onto a page without an annotation. I do not think it is needed to cover the faulty branch, as the main feature of this PR is renaming variables for readability.

[stefan6419846]

So why aren't you simplifying the test then to only cover the relevant parts for this PR?

[j-t-1]

So why aren't you simplifying the test then to only cover the relevant parts for this PR?

Added and amended.