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.

Sign up for GitHub

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

Jump to bottom

MAINT: Use consistent method keywords #1187

Closed
mtd91429 opened this issue Jul 31, 2022 · 5 comments
Closed

MAINT: Use consistent method keywords #1187

mtd91429 opened this issue Jul 31, 2022 · 5 comments
Assignees
@MartinThoma
Labels
is-maintenance Anything that is just internal: Simplifying code, syntax changes, updating docs, speed improvements

Comments

@mtd91429
Copy link
Contributor

mtd91429 commented Jul 31, 2022 •
edited by MartinThoma

Explanation

As PyPDF2 has evolved, some of the internal naming conventions have not been consistent. One recently addressed issue was the outline/bookmark component; I think that was the largest example. As I have been exploring the code base, I have noticed others. I propose that these keywords be systematically implemented across the library and that the deprecation process utilizes an internally consistent process.

Below are the list of keywords that I can see which have inconsistent implementation (only including non-deprecated methods):

✔️ pagenum, page_number, pageNumber (see #1365)

  • pagenum is used in:
    • PdfWriter.addBookmark
    • PdfWriter.add_outline_item
    • PdfWriter.add_named_destination
    • PdfWriter.add_uri
    • PdfWriter.add_link
  • pageNumber is used in:
    • PdfWriter.get_page
  • page_number is used in:
    • PdfWriter.add_annotation
    • PdfWriter.get_page

✔️ pagedest and dest (see #1467)

  • pagedest is used in:
    • PdfWriter.add_link
  • dest is used in:
    • PdfWriter.add_outline_item_destination
    • PdfWriter.add_named_destination_object

✔️ indirect_ref and ido (see #1467 and #1484)

  • indirect_ref is used in:
    • PdfReader._get_page_number_by_indirect
    • PdfReader._flatten
  • indirect_reference is used in:
    • PdfReader._get_object_from_stream
  • ido is used in:
    • PdfWriter.get_object

✔️ pwd and password (see #1483)

  • password is used in:
    • PdfReader.decrypt
  • pwd is used in:
    • PdfWriter.encrypt (as user_pwd, owner_pwd)

Extrinsically inconsistent methods

There are some deprecated methods with keywords that have been changed in the updated function. For example, PdfWriter.addAttachment uses fname and PdfWriter.add_attachment uses filename or PdfWriter.removeImages uses ignoreByteStringObject and PdfWriter.remove_images uses ignore_byte_string_object. These changes are silent.

Intrinsically inconsistent methods

There are some methods which use keywords that are both snake_case and "smooshed" case. For example, PdfReader._read_xref_tables_and_trailers has both startxref and xref_issue_nr,

Inconsistent deprecation implementation

Some of the keywords have been deprecated within the methods. For example, PdfWriter.get_page. Whereas other keywords have been deprecated via decorator; for example, PdfWriter.add_outline_item_dict.

Concluding remarks (for this long winded post)

I don't think all of these examples necessarily need to be changed; I've included them to be as thorough as possible. However, in my opinion, snake_case is best and explicit is better than implied via abbreviation (usually). Please list additional examples that you see and any proposed approaches.
@MartinThoma
Copy link
Member

Wow, amazing work! 😮 Thank you so much 👏👏🤗

I completely agree with your concluding remarks!

@MartinThoma
Copy link
Member

I would completely ignore already deprecated functions / parameters. Think of them as no longer being present in the library. You're seeing a ghost of the past there :-)

@MartinThoma MartinThoma changed the title MAINT: Deploy consistent method keywords MAINT: Use consistent method keywords Jul 31, 2022
@MartinThoma MartinThoma added the is-maintenance Anything that is just internal: Simplifying code, syntax changes, updating docs, speed improvements label Aug 6, 2022
@MartinThoma
Copy link
Member

I have another one: Instead of page_number or start_page_number or similar, PdfMerger.merge uses position

@pubpub-zz
Copy link
Collaborator

pubpub-zz commented Aug 19, 2022 •
edited by MartinThoma

I have another one: Instead of page_number or start_page_number or similar, PdfMerger.merge uses position

Agree, Page_number should be prefered

@MartinThoma MartinThoma mentioned this issue Sep 24, 2022
Merged
@MartinThoma MartinThoma mentioned this issue Nov 21, 2022
Merged
@kygoben kygoben mentioned this issue Dec 2, 2022
Merged
MartinThoma added a commit that referenced this issue Dec 4, 2022
@MartinThoma
MAINT: Use 'page_number' instead of 'pagenum' (#1365)
deb0667 
This PR ensures PyPDF2 uses page_number instead of pagenum as a parameter name.

* It does not touch functions that are deprecated anyway.
* The position of the new parameter is the same as the old one
* The old parameter name is added to ensure that people who use the keyword-parameter don't have a breaking change.

Credits to @mtd91429 who pointed out those inconsistencies in #1187
@Infus3d Infus3d mentioned this issue Dec 10, 2022
Merged
MartinThoma added a commit that referenced this issue Dec 10, 2022
@MartinThoma
MAINT: Consistent parameter/variable names
e369434 
* owner_pwd ➔ owner_password
* user_pwd ➔ user_password

See #1187
@MartinThoma MartinThoma mentioned this issue Dec 10, 2022
Merged
MartinThoma added a commit that referenced this issue Dec 10, 2022
@MartinThoma
MAINT: Consistent parameter/variable names (#1483)
e356a39 
* owner_pwd ➔ owner_password
* user_pwd ➔ user_password

See #1187
MartinThoma pushed a commit that referenced this issue Dec 10, 2022
@kygoben
MAINT: Consistent method keywords (#1467)
2577009 
* indirect_reference ➔ indirect_ref and ido ➔ indirect_ref in PdfReader._get_object_from_stream and PdfWriter.get_object
* dest➔ pagedest and dest_ref ➔ pagedest_ref in PdfWriter.add_outline_item_destination and PdfWriter.add_named_destination_object

See #1187
@MartinThoma MartinThoma mentioned this issue Dec 10, 2022
Merged
MartinThoma pushed a commit that referenced this issue Dec 10, 2022
@Infus3d
MAINT: position ➔ page_number in Merger.merge (#1482)
40086e1 
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
Member

startxref comes from the PDF standard and thus is consistent. The rest was adjusted :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MartinThoma MartinThoma

Labels
is-maintenance Anything that is just internal: Simplifying code, syntax changes, updating docs, speed improvements
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@MartinThoma @mtd91429 @pubpub-zz