Skip to content

MAINT: Refactor Fit / Zoom parameters#1437

Merged
MartinThoma merged 12 commits intomainfrom
refactor-fit
Dec 10, 2022
Merged

MAINT: Refactor Fit / Zoom parameters#1437
MartinThoma merged 12 commits intomainfrom
refactor-fit

Conversation

@MartinThoma
Copy link
Copy Markdown
Member

@MartinThoma MartinThoma commented Nov 19, 2022 •
edited
Loading

Introduce a new PyPDF2.generic.Fit class which captures the type and parameter for how a page should be fit in the viewer (e.g. when clicking on a PDF-internal link).

The class has one method for each fit type which allows users to discover the different types via their IDE, e.g. Fit.xyz(left=123, top=456, zoom=2).

Breaking Change

This PR introduces a breaking change that needs a major version bump. Affected are the following methods:

  • PdfMerger.add_outline_item used fit: FitType and *args: ZoomArgType parameters
  • PdfWriter.add_outline_item used fit: FitType and *args: ZoomArgType parameters
  • AnnotationBuilder.link used fit: FitType = "/Fit" and fit_args: Tuple[ZoomArgType, ...] = tuple() instead.

Instead of having two arguments, we now have only one. To that argument, an object of the new Fit class must be passed.

Why *args is problematic

Using the *args pattern is problematic for two reasons:

  • User-code readability:
    • People cannot use the more expressive keyword-argument syntax in their code for the non-fit parameters
    • People may or may not use the keyword-parameter for the fit parameters
  • Library extensions: PyPDF2 cannot easily add new parameter to those functions; ENH: Add Cloning  #1371 is the latest PR that stumbled over this issue
  • Two parameter for one thing: In general I don't like if we have two parameters for one topic

@MartinThoma MartinThoma force-pushed the refactor-fit branch 3 times, most recently from bf95314 to e0e71b7 Compare November 19, 2022 15:33
@MartinThoma
MAINT: Refactor Fit / Zoom parameters
40cd0e9 
This is a breaking change that needs a major version bump
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 19, 2022 •
edited
Loading

Codecov Report

Base: 94.14% // Head: 94.14% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (c8b65a4) compared to base (ce0e190).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1437   +/-   ##
=======================================
  Coverage   94.14%   94.14%           
=======================================
  Files          30       31    +1     
  Lines        5445     5480   +35     
  Branches     1038     1037    -1     
=======================================
+ Hits         5126     5159   +33     
- Misses        191      193    +2     
  Partials      128      128
Impacted Files Coverage Δ
PyPDF2/_merger.py 93.18% <ø> (ø)
PyPDF2/_reader.py 90.28% <100.00%> (-0.02%) ⬇️
PyPDF2/_writer.py 89.55% <100.00%> (ø)
PyPDF2/generic/__init__.py 100.00% <100.00%> (ø)
PyPDF2/generic/_annotations.py 100.00% <100.00%> (ø)
PyPDF2/generic/_data_structures.py 95.32% <100.00%> (-0.31%) ⬇️
PyPDF2/generic/_fit.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MartinThoma MartinThoma marked this pull request as ready for review November 19, 2022 18:08
@MartinThoma
Copy link
Copy Markdown
Member Author

MartinThoma commented Nov 19, 2022

@pubpub-zz I would introduce this breaking change before #1371. Then you don't need a breaking change in that PR anymore, right?

@MartinThoma
Copy link
Copy Markdown
Member Author

MartinThoma commented Nov 19, 2022

@pubpub-zz @MasterOdin What do you think about this proposal?

pubpub-zz
pubpub-zz reviewed Nov 20, 2022
View reviewed changes
Comment thread PyPDF2/_merger.py
Comment thread PyPDF2/_merger.py Outdated
Comment thread PyPDF2/generic/_fit.py Outdated
@MasterOdin
Copy link
Copy Markdown
Member

MasterOdin commented Nov 21, 2022

Is there value in allowing fit to be specified as a string for the types that don't require any additional arguments (e.g. /Fit)? Not sure how prevalent usage was for setting this to know how much of a breaking change this is.

If this breaking change does happen, does that mean all of the various deprecated functions and things will be removed as well?

@MartinThoma
Copy link
Copy Markdown
Member Author

MartinThoma commented Nov 21, 2022

If this breaking change does happen, does that mean all of the various deprecated functions and things will be removed as well?

Good point. Yes, I would proceed with the deprecations then: https://pypdf2.readthedocs.io/en/latest/dev/deprecations.html#how-pypdf2-deprecates-features
I was thinking about adjusting the procedure:

  1. x.y.(z+1): Add a DeprecationWarning (currently we use PendingDeprecationWarning
  2. (x+1).0.0: Remove / change the code in the breaking way, but keep/add warning / log messages. Here we can help people who didn't look at the warnings before
  3. (x+2).0.0: Also remove the warnings - otherwise the code will be pretty bloated at some point.

I am also thinking about finishing some non-breaking changes in PyPDF2. Work on #1187 , finish the work in #1371 .

Then make a cut, make get rid of all of the deprecation warnings / deprecated methods, and move the project to pypdf (without the 2).

For pypdf I would already have ownership on PyPI. We could also think about moving to pdf: idin/pdf#2
I'm pretty confident that we would get the project. I'm just not sure if it would be the best for the ecosystem. In the end pypdf is pretty easy to google, but PDF is just too generic.

@MartinThoma MartinThoma added this to the PyPDF2==3.0.0 milestone Dec 10, 2022
@MartinThoma MartinThoma added the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Dec 10, 2022
@MartinThoma MartinThoma merged commit 7633477 into main Dec 10, 2022
@MartinThoma MartinThoma deleted the refactor-fit branch December 10, 2022 19:54
@MartinThoma MartinThoma removed the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Dec 10, 2022
MartinThoma added a commit that referenced this pull request Dec 22, 2022
@MartinThoma
REL: 3.0.0
3214dde 
BREAKING CHANGES:
-  Deprecate features with PyPDF2==3.0.0 (#1489)
-  Refactor Fit / Zoom parameters (#1437)

New Features (ENH):
-  Add Cloning  (#1371)
-  Allow int for indirect_reference in PdfWriter.get_object (#1490)

Documentation (DOC):
-  How to read PDFs from S3 (#1509)
-  Make MyST parse all links as simple hyperlinks (#1506)
-  Changed 'latest' for 'stable' generated docs (#1495)
-  Adjust deprecation procedure (#1487)

Maintenance (MAINT):
-  Use typing.IO for file streams (#1498)

[Full Changelog](2.12.1...3.0.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pubpub-zz pubpub-zz pubpub-zz left review comments

@MasterOdin MasterOdin Awaiting requested review from MasterOdin

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

PyPDF2==3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MartinThoma @MasterOdin @pubpub-zz