search

py-pdf / pypdf

A pure-python PDF library capable of splitting, merging, cropping, and transforming the pages of PDF files
https://pypdf.readthedocs.io/en/latest/
Other
8.41k stars 1.42k forks source link

MAINT: Update version of deprecation_with_replacement #2861

Closed j-t-1 closed 1 month ago

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.43%. Comparing base (a7d5c8d) to head (19c9c6e). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2861 +/- ## ======================================= Coverage 96.43% 96.43% ======================================= Files 52 52 Lines 8726 8726 Branches 1721 1721 ======================================= Hits 8415 8415 Misses 182 182 Partials 129 129 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stefan6419846 commented 2 months ago

Seems like we missed them during the cleanup, as they should already have been removed as well - they already were errors if I am not mistaken. Thus I do not really consider this change to be correct. Depending on whether we yank the 5.0.0 release, this might go into the fixed 5.0.0 release with a complete cleanup.

j-t-1 commented 2 months ago

If it was not an __init__.py file would this change be okay?

Is "AnnotationBuilder" okay to be in __all__?

stefan6419846 commented 2 months ago

No, I am questioning why we are still delaying the deprecation which - if I understand correctly, should already have happened by completely removing the offending code.

j-t-1 commented 2 months ago

These were deprecate_with_replacement in 4.0.0 and made deprecation_with_replacement in #2813, but the 4.0.0 should have been increased to 5.0.0. However, on further consideration, because it is an __init__.py file I think it needs to be removed from this file. We are agreeing for its removal but for different reasons.

stefan6419846 commented 1 month ago

What is your plan on continuing with this? If ever, we should probably update the version number to 6.0 then to indicate that this is gone in version 5.0?

j-t-1 commented 1 month ago

I think it should be 5.0.0 if I understand the deprecation process. Previously I said being an __init__.py file is an issue, but on reflection this is okay as these member functions are not being used, only defined.

pubpub-zz commented 1 month ago

Unless I'm wrong :

in release 4.x.x, the process was not properly applied ( we should have moved to deprecation)

in 5.0.0, the mistake has been fixed and we moved to `deprecation_with_replacement( (....), "4.0.0") maybe I should have declared 5.0.0.

in 6.0.0, we will completely removed these functions. OK for me to validate the PR