Skip to content

MAINT: position ➔ page_number in Merger.merge#1482

Merged
MartinThoma merged 22 commits intopy-pdf:mainfrom
Infus3d:1187-consistent-method-keywords
Dec 10, 2022
Merged

MAINT: position ➔ page_number in Merger.merge#1482
MartinThoma merged 22 commits intopy-pdf:mainfrom
Infus3d:1187-consistent-method-keywords

Conversation

@Infus3d
Copy link
Copy Markdown
Contributor

@Infus3d Infus3d commented Dec 10, 2022 •
edited by MartinThoma
Loading

Changed method parameter name for consistency as mentioned in #1187

  • position ➔ page_number in PdfMerger.merge

Improved the deprecation message of:

  • dest ➔ page_destination in PdfWriter.add_outline_item_destination and PdfWriter.add_named_destination_object
  • user_pwd ➔ user_password and owner_pwd ➔ owner_password in PdfWriter.encrypt

@MartinThoma
Copy link
Copy Markdown
Member

MartinThoma commented Dec 10, 2022

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
Copy Markdown
Member

MartinThoma commented Dec 10, 2022 •
edited
Loading

I'll merge #1467 before this one

@Infus3d
Copy link
Copy Markdown
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
Copy Markdown
Member

MartinThoma commented Dec 10, 2022

image

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

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 10, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Member

MartinThoma commented Dec 10, 2022 •
edited
Loading

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
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
@MartinThoma MartinThoma changed the title Consistent method keywords MAINT: position ➔ page_number in Merger.merge Dec 10, 2022
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
MartinThoma
MartinThoma reviewed Dec 10, 2022
View reviewed changes
@MartinThoma MartinThoma merged commit 40086e1 into py-pdf:main Dec 10, 2022
MartinThoma added a commit that referenced this pull request Dec 10, 2022
@MartinThoma
REL: 2.12.1
ce0e190 
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

Reviewers

@MartinThoma MartinThoma MartinThoma left review comments

Assignees

No one assigned

Labels

soon PRs that are almost ready to be merged, issues that get solved pretty soon

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Infus3d @MartinThoma