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

DEP: Class, variable, and module names #867

Merged
merged 42 commits into from May 22, 2022
Merged

DEP: Class, variable, and module names #867

merged 42 commits into from May 22, 2022

Conversation

MartinThoma
Copy link
Member

@MartinThoma MartinThoma commented May 8, 2022

Deprecation Announcements: The following will be deprecated with PyPDF2 2.0.0:

  • Python 3.5 and older
  • formatWarning : No deprecation warning was added as this leads to a recursion error
  • PdfFileReader: The arguments warndest and overwriteWarnings will be removed. The new default behavior is overwriteWarnings=False and warndest=None
  • PdfFileMerger: The argument overwriteWarnings will be removed. The new default behavior is overwriteWarnings=False.
  • PyPDF2.merger.OutlinesObject will be removed without replacement.
  • PyPDF2.pdf: The pdf module will be removed. Import directly from PyPDF2 instead.
  • PyPDF2.utils: The utils module will become private.

Core class renames

  • PdfFileReader➔ PdfReader
  • PdfFileWriter➔ PdfWriter
  • PdfFileMerger➔ PdfMerger

@MartinThoma
Copy link
Member Author

This is a preparation for #859

@codecov
Copy link

codecov bot commented May 8, 2022

Codecov Report

Merging #867 (ea38899) into main (a791ef1) will decrease coverage by 5.27%.
The diff coverage is 72.07%.

@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
- Coverage   82.46%   77.19%   -5.28%     
==========================================
  Files          16       17       +1     
  Lines        3776     4328     +552     
  Branches      802      819      +17     
==========================================
+ Hits         3114     3341     +227     
- Misses        495      812     +317     
- Partials      167      175       +8     
Impacted Files Coverage Δ
PyPDF2/pdf.py 0.00% <0.00%> (-100.00%) ⬇️
PyPDF2/utils.py 0.00% <0.00%> (-93.61%) ⬇️
PyPDF2/merger.py 64.02% <61.29%> (-3.31%) ⬇️
PyPDF2/generic.py 79.32% <63.52%> (-6.50%) ⬇️
PyPDF2/xmp.py 51.81% <65.00%> (+1.54%) ⬆️
PyPDF2/_writer.py 79.81% <69.58%> (-8.93%) ⬇️
PyPDF2/_page.py 73.22% <70.37%> (-5.90%) ⬇️
PyPDF2/_reader.py 76.60% <73.00%> (-4.62%) ⬇️
PyPDF2/filters.py 77.14% <80.00%> (-0.32%) ⬇️
PyPDF2/_utils.py 91.42% <91.42%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a791ef1...ea38899. Read the comment docs.

Core classes:

* PdfFileReader➔ PdfReader
* PdfFileWriter➔ PdfWriter
* PdfFileMerger➔ PdfMerger
@pubpub-zz
Copy link
Collaborator

@MartinThoma, May I ask you a question : I do agree that the new names may be more appropriate and in accordance with snake case style, but what not leave the old style with the warning in the future : there will no drawbacks for PyPDF2 development keeping them to keep backward compatibility with the warning and a very limited support

@MartinThoma
Copy link
Member Author

[why] not leave the old style with the warning in the future : there will no drawbacks for PyPDF2 development keeping them to keep backward compatibility with the warning and a very limited support

I'm currently wondering if I really want to do this style change at all 😄 There are so many tiny places ... that is way more work than I expected (mainly due to the fact that I need to add the deprecation warnings).

Yes, I was also thinking to keep the parts with the warnings for a while - not measured in PyPDF2 versions, but in calendar time. For example, for a year after the 2.0.0 version is on PyPI.

However, I did have a look at a a lot of StackOverflow questions tagged with PyPDF2. Most people seem to use only very basic features of PyPDF2 and not go into more "internal" topics. The most common use-cases seem to be: (1) Text Extraction (2) simple merges without transformations (3) extracting images (4) getting the number of pages of a document. Hence I would not expect too many hickups / breaking things a lot. But there might be a lot hidden that I'm not aware of.

I want to drop the deprecation warnings eventually as this clutters the code-base, but I also don't want to break existing code. I'm 90% decided by now that I keep the old names for ~1 year after the 2.0.0 release.

What do you think about this idea?

@MartinThoma
Copy link
Member Author

@pubpub-zz Looking at utils.py, it seems as if all of those functions are probably not used by PyPDF2 users (only by PyPDF2 developers). Hence I think about making the module private (utils.py -> _utils.py). Do you also think so.

@pubpub-zz
Copy link
Collaborator

I'm currently wondering if I really want to do this style change at all 😄 There are so many tiny places ... that is way more work than I expected (mainly due to the fact that I need to add the deprecation warnings).

I've started this work previously to match pylint and it has been a hudge job not finish.
I also agree that we should focus(only?) in providing a snake case style API for the documented functions

want to drop the deprecation warnings eventually as this clutters the code-base, but I also don't want to break existing code. I'm 90% decided by now that I keep the old names for ~1 year after the 2.0.0 release.

What About using @MasterOdin's proposal (#684 (comment)) to provide both functions names but encoding the snake case style? warning can still be introduced there.

Also I do like your idea to about PendingDeprecationWarning,but I've just discovered that this warning is nearly all the time not reported. perhaps we should use our our warning and provide a function set the warning filters to show/hide them

@pubpub-zz
Copy link
Collaborator

@pubpub-zz Looking at utils.py, it seems as if all of those functions are probably not used by PyPDF2 users (only by PyPDF2 developers). Hence I think about making the module private (utils.py -> _utils.py). Do you also think so.

agreed

@pubpub-zz
Copy link
Collaborator

What About using @MasterOdin's proposal (#684 (comment)) to provide both functions names but encoding the snake case style?

I don't understand the question. Isn't that what I'm doing in this PR?

You seem to recode manually the alternative names for both. The proposal was to do this 'conversion' at the flight and by code.

@pubpub-zz
Copy link
Collaborator

Also I do like your idea to about PendingDeprecationWarning,but I've just discovered that this warning is nearly all the time not reported. perhaps we should use our our warning and provide a function set the warning filters to show/hide them

You mean something like this?

class PyPDF2DeprecationWarning(DeprecationWarning):
    pass

I haven't seen that before 🤔

provide a function set the warning filters to show/hide them

I don't see how this would solve the issue that people might miss it. If people enable warnings, they are shown. The problem occurs only when people have/leave the warnings disabled.

I'm pretty heavily editing a lot of StackOverflow questions, so I hope that people see/like the new style and more naturally there. The changelog + the major version change should also make it obvious that there might be breaking changes (and for the changes with most potential for issues I will keep it for longer than the 2.0.0 release; I'm still thinking about how to communicate this properly).

I agree my idea is not good.

@MartinThoma
Copy link
Member Author

You seem to recode manually the alternative names for both. The proposal was to do this 'conversion' at the flight and by code.

Yes, I was. How would you do it differently? (I'll read master odins comment again tomorrow to see what I missed :-))

@MartinThoma MartinThoma marked this pull request as ready for review May 22, 2022 10:43
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