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

Rename create_viewer_preference #2167

Closed
wants to merge 2 commits into from
Closed

Conversation

marcstober
Copy link
Contributor

@marcstober marcstober commented Sep 7, 2023

Proposed change to not include create_viewer_preference in the public API. Also fix some docstring typos.

See #2144

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (c587cee) 94.34% compared to head (e663e53) 94.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2167      +/-   ##
==========================================
+ Coverage   94.34%   94.36%   +0.01%     
==========================================
  Files          43       43              
  Lines        7572     7572              
  Branches     1488     1488              
==========================================
+ Hits         7144     7145       +1     
  Misses        263      263              
+ Partials      165      164       -1     
Files Changed Coverage Δ
pypdf/_writer.py 88.34% <100.00%> (+0.08%) ⬆️

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

@stefan6419846
Copy link
Collaborator

Could you please replace the PR title with some more meaningful one? See https://pypdf.readthedocs.io/en/latest/dev/intro.html#commit-messages for additional information.

@marcstober
Copy link
Contributor Author

@stefan6419846 sure, this is related to another recent change @pubpub-zz and @MartinThoma have been working on, so it would be good to get their feedback too. I'm not sure if this should also be ENH or STY or something else?

@pubpub-zz
Copy link
Collaborator

pubpub-zz commented Sep 7, 2023

I'm not fond of creating automatically the view_preference object:
a) it changes the behavior of the function between PdfReader and PdfWriter of viewer_preferences()
b) if you really want to do the action in "one instruction" you just have to do it that way:
v = writer.viewer_preferences or writer.create_viewer_preference()

@MartinThoma MartinThoma changed the title Proposed change to #2144 Include create_viewer_preference in the public API Sep 10, 2023
@MartinThoma MartinThoma changed the title Include create_viewer_preference in the public API Not include create_viewer_preference in the public API Sep 10, 2023
@marcstober marcstober changed the title Not include create_viewer_preference in the public API Rename create_viewer_preference Sep 12, 2023
@marcstober
Copy link
Contributor Author

@pubpub-zz ok, I updated this pull request with just the rename discussed here: #2105 (comment)

@MartinThoma I think this is ready to merge if it's OK with you and @pubpub-zz now?

I'm not fond of creating automatically the view_preference object: a) it changes the behavior of the function between PdfReader and PdfWriter of viewer_preferences() b) if you really want to do the action in "one instruction" you just have to do it that way: v = writer.viewer_preferences or writer.create_viewer_preference()

@pubpub-zz
Copy link
Collaborator

@marcstober
this PR is now obsolete and overwritten by #2190
isn't it ?

@marcstober
Copy link
Contributor Author

@pubpub-zz yes, I'd originally thought this could be merged first

@pubpub-zz
Copy link
Collaborator

duplicate of #2190

@pubpub-zz pubpub-zz closed this Sep 13, 2023
@pubpub-zz
Copy link
Collaborator

less work for @MartinThoma already over busy 😉

@marcstober marcstober deleted the viewerprefs branch September 18, 2023 14:27
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.

3 participants