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: indirect_ref/ido ➔ indirect_reference, dest➔ page_destination #1467

Merged
merged 17 commits into from Dec 10, 2022

Conversation

kygoben
Copy link
Contributor

@kygoben kygoben commented Dec 2, 2022

From issue #1187 used the descriptions from @mtd91429 to make a few modifications to PdfWriter and PdfReader

  • indirect_refindirect_reference and idoindirect_reference in PdfReader._get_object_from_stream and PdfWriter.get_object
  • destpage_destination and dest_refpage_destination_ref in PdfWriter.add_outline_item_destination and PdfWriter.add_named_destination_object

…dest_ref in pdfwriter.add_outline_item_destination and pdfwriter.add_named_destination_object
@kygoben kygoben marked this pull request as draft December 2, 2022 04:54
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Base: 93.85% // Head: 93.76% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (ea4a29d) compared to base (e356a39).
Patch coverage: 74.07% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1467      +/-   ##
==========================================
- Coverage   93.85%   93.76%   -0.09%     
==========================================
  Files          30       30              
  Lines        5464     5470       +6     
  Branches     1046     1048       +2     
==========================================
+ Hits         5128     5129       +1     
- Misses        202      206       +4     
- Partials      134      135       +1     
Impacted Files Coverage Δ
PyPDF2/_writer.py 87.42% <65.00%> (-0.65%) ⬇️
PyPDF2/_reader.py 90.30% <100.00%> (ø)

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.

@MartinThoma
Copy link
Member

@kygoben Thank you for approaching this!

When we make those changes, we want to make them first in a non-breaking way. That means people who use the method with keyword parameters should receive a warning (but not an exception) that their usage is deprecated.

We also need to add this to the migration guide: https://pypdf2.readthedocs.io/en/latest/user/migration-1-to-2.html

Finally, we should typically use the long form. That means:

  • Instead of indirect_ref I would like to have indirect_reference
  • pagedest should rather be page_destination

See #1365 for a similar PR

@kygoben
Copy link
Contributor Author

kygoben commented Dec 6, 2022

Hi @MartinThoma, I am a bit unsure how to completely approach your comments. For instance, you said that the migration guide would need to be updated. Where can I find this, and how should this be updated? And the second one is to give people a warning when using the keywords. Thank you for your help :)

@kygoben
Copy link
Contributor Author

kygoben commented Dec 7, 2022

Not sure what I did incorrectly in these last modifications. Could someone give me some pointers to fix the test outcomes?
Thanks,
Kyle

@MartinThoma
Copy link
Member

I'll have a look at the weekend. I was at a company event this week and moving/getting married last week :-)

@MartinThoma
Copy link
Member

If you look into _page.py you can see in the __init__ method some indirect_ref that need to be changed to indirect_reference

@MartinThoma
Copy link
Member

It would also be good to add a property indirect_ref that gives a deprecation warning + returns the indirect_reference. Just in case people use it in their code.

@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 added this to the PyPDF2==3.0.0 milestone Dec 10, 2022
@MartinThoma
Copy link
Member

I'll add this either before the PyPDF2==3.0.0 release or earlier.

@MartinThoma MartinThoma changed the title MAINT: Consistent method keywords #1187 MAINT: Consistent method keywords Dec 10, 2022
PyPDF2/_writer.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinThoma MartinThoma left a comment

Choose a reason for hiding this comment

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

Rename Page.indirect_reference temporarily back to Page.indirect_ref to fix the PR

PyPDF2/_reader.py Outdated Show resolved Hide resolved
PyPDF2/_reader.py Outdated Show resolved Hide resolved
PyPDF2/_reader.py Outdated Show resolved Hide resolved
tests/test_writer.py Outdated Show resolved Hide resolved
tests/test_writer.py Outdated Show resolved Hide resolved
This change should allow me to merge this PR.
PyPDF2/_writer.py Outdated Show resolved Hide resolved
PyPDF2/_writer.py Outdated Show resolved Hide resolved
@MartinThoma MartinThoma marked this pull request as ready for review December 10, 2022 14:47
@MartinThoma MartinThoma merged commit 2577009 into py-pdf:main Dec 10, 2022
@MartinThoma MartinThoma removed the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Dec 10, 2022
@MartinThoma MartinThoma changed the title MAINT: Consistent method keywords MAINT: indirect_ref/ido ➔ indirect_reference, dest➔ page_destination 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants