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

BUG: Improve PdfWriter handing of context manager #2913

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

pubpub-zz
Copy link
Collaborator

closes #2912

pypdf/_writer.py Outdated Show resolved Hide resolved
pypdf/_writer.py Outdated Show resolved Hide resolved
pypdf/_writer.py Outdated Show resolved Hide resolved
pypdf/_writer.py Outdated Show resolved Hide resolved
@@ -166,6 +171,7 @@ class PdfWriter(PdfDocCommon):

def __init__(
self,
*args: Any,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only as unnamed argument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the unnamed parameter will be assigned to fileobj or clone_from. I do not really understand your question

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been thinking about what would be the best way to handle such cases and whether we really need this whole PR - IMHO keyword arguments should always be preferred over positional arguments, while keyword arguments have always worked correctly.

My recommendation would be to keep the old behavior, but deprecate unnamed arguments and make the constructor keyword-only in the future. This way, we force users to clearly express their intents without having to introduce further magic on our side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it as it is now : first for many cases it is easier to not need to add a parameter name when typing. second we have this syntax for some time. Let's open a discussion about it and will see what is the feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your proposed approach, we get some sort of mixed handling which I consider even more harmful: Previously, you would be able to pass all parameters as positional ones, but allowing for keyword-based and mixed ones as well. The current state of this PR will drop all positional parameters except the first one. This is an undocumented breaking change which would need a deprecation process as well - thus switching to keyword-only arguments with a deprecation process is not much different while enforcing users to actually think of what they want to do.

The amount of characters to type should not really matter as well. A few libraries already migrated to keyword-only arguments in the past as this makes everything more readable. Additionally, every modern IDE (and even some regular text editors) provide support for autocompletion based upon the method signature.

pypdf/_writer.py Outdated Show resolved Hide resolved
tests/test_writer.py Outdated Show resolved Hide resolved
pubpub-zz and others added 7 commits October 20, 2024 14:25
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.45%. Comparing base (80c3939) to head (2f98e6c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2913   +/-   ##
=======================================
  Coverage   96.44%   96.45%           
=======================================
  Files          52       52           
  Lines        8728     8736    +8     
  Branches     1589     1592    +3     
=======================================
+ Hits         8418     8426    +8     
  Misses        182      182           
  Partials      128      128           

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

pypdf/_writer.py Outdated Show resolved Hide resolved
pypdf/_writer.py Outdated Show resolved Hide resolved
pubpub-zz and others added 8 commits October 20, 2024 16:22
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
Copy link
Collaborator

@stefan6419846 stefan6419846 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While #2913 (review) is not addressed accordingly (without just marking it as resolved), I do not consider this to be ready for merging.

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.

Cloning errors when using context manager
2 participants