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

MAINT: position ➔ page_number in Merger.merge #1482

Merged
merged 22 commits into from Dec 10, 2022

Conversation

Infus3d
Copy link
Contributor

@Infus3d Infus3d commented Dec 10, 2022

Changed method parameter name for consistency as mentioned in #1187

  • positionpage_number in PdfMerger.merge

Improved the deprecation message of:

  • destpage_destination in PdfWriter.add_outline_item_destination and PdfWriter.add_named_destination_object
  • user_pwduser_password and owner_pwdowner_password in PdfWriter.encrypt

@MartinThoma
Copy link
Member

Thank you for the pr 🙏

I recommend to use pre-commit as it fixes some style issues:

# install pre-commit
pip install pre-commit

# within the PyPDF2 repository, install the pre-commit hooks:
pre-commit install

@MartinThoma
Copy link
Member

MartinThoma commented Dec 10, 2022

I'll merge #1467 before this one

@Infus3d
Copy link
Contributor Author

Infus3d commented Dec 10, 2022

I thought the tests were running fine locally. Do you have an idea why the check is failing by chance?

@MartinThoma MartinThoma changed the title 1187 consistent method keywords Consistent method keywords Dec 10, 2022
@MartinThoma
Copy link
Member

image

this is a breaking change as you moved the position of the argument. Not everybody uses keyword-arguments.

@codecov
Copy link

codecov bot commented Dec 10, 2022

Codecov Report

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

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

❗ Current head 7de7c4b differs from pull request most recent head 3d1deb2. Consider uploading reports for the commit 3d1deb2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1482      +/-   ##
==========================================
+ Coverage   94.12%   94.14%   +0.01%     
==========================================
  Files          30       30              
  Lines        5447     5445       -2     
  Branches     1039     1038       -1     
==========================================
- Hits         5127     5126       -1     
  Misses        191      191              
+ Partials      129      128       -1     
Impacted Files Coverage Δ
PyPDF2/_merger.py 93.18% <100.00%> (-0.03%) ⬇️
PyPDF2/_writer.py 89.55% <100.00%> (+0.13%) ⬆️

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.

@Infus3d
Copy link
Contributor Author

Infus3d commented Dec 10, 2022

I changed the order back, it should be fine now. Although the workaround is a bit cheap, it is the only way I can think of. 😅

@MartinThoma
Copy link
Member

MartinThoma commented Dec 10, 2022

Thank you @Infus3d !

edit: I've just fixed the merge conflicts.

@MartinThoma MartinThoma added the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Dec 10, 2022
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
@MartinThoma MartinThoma changed the title Consistent method keywords MAINT: position ➔ page_number in Merger.merge Dec 10, 2022
PyPDF2/_merger.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_merger.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_merger.py Outdated Show resolved Hide resolved
@MartinThoma MartinThoma merged commit 40086e1 into py-pdf:main Dec 10, 2022
MartinThoma added a commit that referenced this pull request Dec 10, 2022
Documentation (DOC)
-  Deduplicate extract_text docstring (#1485)
-  How to cite PyPDF2 (#1476)

Maintenance (MAINT)
  Consistency changes:
  -  indirect_ref/ido ➔ indirect_reference, dest➔ page_destination (#1467)
  -  owner_pwd/user_pwd ➔ owner_password/user_password (#1483)
  -  position ➜ page_number in Merger.merge (#1482)
  -  indirect_ref ➜ indirect_reference (#1484)

[Full Changelog](2.12.0...2.12.1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
soon PRs that are almost ready to be merged, issues that get solved pretty soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants