Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.43%. Comparing base (018a52e) to head (e684a13).
⚠️ Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3310   +/-   ##
=======================================
  Coverage   97.43%   97.43%           
=======================================
  Files          55       55           
  Lines       10005    10016   +11     
  Branches     1837     1841    +4     
=======================================
+ Hits         9748     9759   +11     
  Misses        149      149           
  Partials      108      108           

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

Fixed deprecate_with_replacement. Ready for code review.

Could you please elaborate if we have any test in place to properly check that the remove_orphans parameter is properly respected and where? #3691 has been opened recently and I want to avoid merging this first when your PR already covers all relevant aspects.

Could you please elaborate if we have any test in place to properly check that the remove_orphans parameter is properly respected and where?

Lines 2578 to 2585 do this check.

Why is this test in a section which checks that warnings are emitted? As far as I can see, this always sets remove_orphans, which actually is the inverse of what I am asking for.

Additionally, from the code it is not directly obvious what is checked here due to some "arbitrary" length checks - could you please try to improve it to make it more obvious what happened? Maybe there are some unreferenced dummy objects we could check for explicitly?

Why is this test in a section which checks that warnings are emitted? As far as I can see, this always sets remove_orphans, which actually is the inverse of what I am asking for.

It does the warning and then removes duplicates. Sorry I do not understand what you mean. Do you mean split it in to two, one just tests a warning and then one tests deduplication?

Additionally, from the code it is not directly obvious what is checked here due to some "arbitrary" length checks - could you please try to improve it to make it more obvious what happened? Maybe there are some unreferenced dummy objects we could check for explicitly?

This is a good idea. How about loop over writer._objects, take the id()of each and then count using a dict? Do this before and after and then ensure any values that were greater than one are now equal to one.

Do you mean split it in to two, one just tests a warning and then one tests deduplication?

Yes, because I usually tend to remove all warning code where possible when preparing a new major release, thus having this separated making it easier to maintain.

Regarding the proposed test, doing something like this should be sufficient in theory:

def test_remove_orphans():
    writer = PdfWriter(clone_from=RESOURCE_ROOT / "crazyones.pdf")
    writer._add_object(DictionaryObject({}))
    dictionary_object = DictionaryObject({NameObject("/Testing"): NameObject("/UniqueNameForTesting")})
    reference = writer._add_object(dictionary_object)

    writer.compress_identical_objects(remove_orphans=False)
    assert writer.get_object(reference) == dictionary_object

    writer.compress_identical_objects(remove_orphans=True)
    with pytest.raises(AssertionError):
        writer.get_object(reference)

The AssertionError might be replaced by a proper exception here - mypy is the wrong justification here.

Is test_remove_orphans the test of the deprecation? If yes, it should have the warnings, if not it should use the renamed values.

Can you clarify the names of the test functions that test deprecation and test compression?

No, this would be the one for the new way. I just used the old names as this is what my local testing environment for quick verification has.