Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STY: Reorder methods in PdfReader and PdfWriter #2654

Closed
wants to merge 1 commit into from

Conversation

j-t-1
Copy link
Contributor

@j-t-1 j-t-1 commented May 19, 2024

Reorder to make more logical, and try to put similar methods in the classes in similar order. Put repr_mimebundle last as it goes less with the other methods.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Reorder to make more logical, and try to put similar methods in the classes in similar order. Put _repr_mimebundle_ last as it goes less with the other methods.
Copy link

codecov bot commented May 19, 2024

Codecov Report

Attention: Patch coverage is 92.36641% with 10 lines in your changes missing coverage. Please review.

Project coverage is 94.97%. Comparing base (c227b0c) to head (97c44b4).
Report is 71 commits behind head on main.

Files Patch % Lines
pypdf/_reader.py 92.15% 4 Missing and 4 partials ⚠️
pypdf/_writer.py 93.10% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2654   +/-   ##
=======================================
  Coverage   94.97%   94.97%           
=======================================
  Files          50       50           
  Lines        8331     8331           
  Branches     1669     1669           
=======================================
  Hits         7912     7912           
  Misses        260      260           
  Partials      159      159           

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

@MartinThoma
Copy link
Member

I'm very skeptical of reordering methods. It's not clear to be if this has any advantage, but the disadvantage is obvious: It becomes very hard to see which PR/commit introduced which line of code.

Additionally, this PR will likely introduce merge conflicts with other PRs + it will be extremely cumbersome to review.

I would close it. What are your thoughts @stefan6419846 / @pubpub-zz ?

@stefan6419846
Copy link
Collaborator

For me it does not really matter how the methods are ordered as the API docs and autocompletion of IDEs orders them anyway. And yes, this significantly complicates blaming.

@pubpub-zz
Copy link
Collaborator

I'm very skeptical of reordering methods. It's not clear to be if this has any advantage, but the disadvantage is obvious: It becomes very hard to see which PR/commit introduced which line of code.

Additionally, this PR will likely introduce merge conflicts with other PRs + it will be extremely cumbersome to review.

I would close it. What are your thoughts @stefan6419846 / @pubpub-zz ?

When I did the PR to have common code between PdfReader and PdfWriter, Some orders could have been optimized. I was open to improve this order however, I missed the impact on conflicts resolving and code blaming. I now agree that this will have bad impact.😔

@j-t-1
Copy link
Contributor Author

j-t-1 commented May 20, 2024

I'm very skeptical of reordering methods. It's not clear to be if this has any advantage, but the disadvantage is obvious: It becomes very hard to see which PR/commit introduced which line of code.

This is a fair criticism and has much weighting. At a minimum method is_encrypted should be moved below __init__, as this usually goes first. Putting repr_mimebundle last as it goes less with the other methods if it does not alter any other lines would be good.

When completely new complementary methods are added to both PdfReader and PdfWriter we should try and put them at the end of the class, so the new ones have the same order.

There is a need for having a table (in the documentation say) with rows showing analog methods from PdfReader and PdfWriter (with missing values when analogs do not exist).

@stefan6419846
Copy link
Collaborator

There is a need for having a table (in the documentation say) with rows showing analog methods from PdfReader and PdfWriter (with missing values when analogs do not exist).

In my opinion it should be sufficient to enable the class docs for PdfDocCommon to indicate common attributes there. Manually maintaining something like this should not be necessary. But this is out of scope for this PR in my opinion.

@stefan6419846
Copy link
Collaborator

Closing due to the above discussions to favor a clean history over a perfect order.

@j-t-1
Copy link
Contributor Author

j-t-1 commented May 20, 2024

There is a need for having a table (in the documentation say) with rows showing analog methods from PdfReader and PdfWriter (with missing values when analogs do not exist).

In my opinion it should be sufficient to enable the class docs for PdfDocCommon to indicate common attributes there. Manually maintaining something like this should not be necessary. But this is out of scope for this PR in my opinion.

Did not think of this option. This is a good idea.

@j-t-1 j-t-1 deleted the reorder branch May 20, 2024 19:12
@j-t-1
Copy link
Contributor Author

j-t-1 commented Sep 18, 2024

The need for a clean ordering makes sense. I think the only exception is classes traditionally having __init__ first.
@pubpub-zz would you be okay puting this first in ViewerPreferences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants