Skip to content

MAINT: Add wrapper function for PendingDeprecationWarnings#928

Merged
MartinThoma merged 1 commit intomainfrom
maint-deprecations
Jun 1, 2022
Merged

MAINT: Add wrapper function for PendingDeprecationWarnings#928
MartinThoma merged 1 commit intomainfrom
maint-deprecations

Conversation

@MasterOdin
Copy link
Copy Markdown
Member

@MasterOdin MasterOdin commented May 30, 2022

PR updates all deprecation warnings in the codebase to go through two wrapper functions, to make it easy to keep the message unified, ensure proper stacklevel used, and that the same type of warning is raised, overall ensuring we're not doing a lot of repeating ourselves. All three of those were different in various places across the codebase, leading to a bit of a confusing experience. The wrapper functions were originally accidentally added in #913, though were unused till now.

Signed-off-by: Matthew Peveler <matt.peveler@gmail.com>


def deprecate(msg: str, stacklevel: int = 3) -> None:
warnings.warn(msg, PendingDeprecationWarning, stacklevel=stacklevel)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One note is that I believe that this should be changed to a DeprecationWarning as they've been deprecated and will be removed in next major version, where PendingDeprecationWarning should be used for functions that will be deprecated in the future, but are not yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with changing it.

As we are now on it, maybe we should specify the process. I would suggest the following if we want to remove a class / function / method. If the version before the change is x.y.z, then:

  1. x.y.(z+1): Add a PendigDeprecationWarning. If there is a replacement, the replacement is also introduced and the warning informs about this change. The docs let users know about the deprecation + the new function. The CHANGELOG informs about it.
  2. (x+1).0.0: The PendingDeprecationWarning is changed to a DeprecationWarning. The CHANGELOG informs about it.
  3. (x+2).0.0: The code is actually removed. The CHANGELOG informs about it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've added those thoughts to a PR: #930

@codecov
Copy link
Copy Markdown

codecov bot commented May 30, 2022

Codecov Report

Merging #928 (69e040b) into main (552767e) will increase coverage by 0.07%.
The diff coverage is 12.57%.

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
+ Coverage   78.25%   78.32%   +0.07%     
==========================================
  Files          16       16              
  Lines        4346     4342       -4     
  Branches      821      821              
==========================================
  Hits         3401     3401              
+ Misses        758      754       -4     
  Partials      187      187              
Impacted Files Coverage Δ
PyPDF2/generic.py 77.77% <1.61%> (ø)
PyPDF2/_writer.py 79.01% <2.70%> (+0.11%) ⬆️
PyPDF2/_reader.py 77.15% <7.14%> (+0.09%) ⬆️
PyPDF2/_merger.py 65.61% <14.28%> (+0.09%) ⬆️
PyPDF2/_page.py 76.44% <36.36%> (-0.06%) ⬇️
PyPDF2/xmp.py 60.62% <50.00%> (-0.21%) ⬇️
PyPDF2/_utils.py 90.67% <75.00%> (+1.02%) ⬆️

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 552767e...69e040b. Read the comment docs.

PendingDeprecationWarning,
stacklevel=2,
)
deprecate(DEPR_MSG.format(old_name, new_name), 4)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nitpic: I try to use keyword arguments when I use literal parameters, e.g.:

Suggested change
deprecate(DEPR_MSG.format(old_name, new_name), 4)
deprecate(DEPR_MSG.format(old_name, new_name), stacklevel=4)

I think this is easier to read.

Copy link
Copy Markdown
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.

Very nice! You killed 700 lines of code with this PR 😍

@MartinThoma
Copy link
Copy Markdown
Member

Feel free to merge whenever you want :-)

@MartinThoma MartinThoma merged commit 5730198 into main Jun 1, 2022
@MartinThoma MartinThoma deleted the maint-deprecations branch June 1, 2022 05:37
MartinThoma added a commit that referenced this pull request Jun 1, 2022
The 2.0.0 release of PyPDF2 includes three core changes:

1. Dropping support for Python 3.5 and older.
2. Introducing type annotations.
3. Interface changes, mostly to have PEP8-compliant names

We introduced a [deprecation process](#930)
that hopefully helps users to avoid unexpected breaking changes.

Breaking Changes(DEP):
- PyPDF2 2.0 requires Python 3.6+. Python 2.7 and 3.5 support were dropped.
- PdfFileReader: The "warndest" parameter was removed
- PdfFileReader and PdfFileMerger no longer have the `overwriteWarnings`
  parameter. The new behavior is `overwriteWarnings=False`.
- merger: OutlinesObject was removed without replacement.
- merger.py ➔ _merger.py: You must import PdfFileMerger from PyPDF2 directly.
- utils:
  * `ConvertFunctionsToVirtualList` was removed
  * `formatWarning` was removed
  * `isInt(obj)`: Use `instance(obj, int)` instead
  * `u_(s)`: Use `s` directly
  * `chr_(c)`: Use `chr(c)` instead
  * `barray(b)`: Use `bytearray(b)` instead
  * `isBytes(b)`: Use `instance(b, type(bytes()))` instead
  * `xrange_fn`: Use `range` instead
  * `string_type`: Use `str` instead
  * `isString(s)`: Use `instance(s, str)` instead
  * `_basestring`: Use `str` instead
  * All Exceptions are now in `PyPDF2.errors`:
    - PageSizeNotDefinedError
    - PdfReadError
    - PdfReadWarning
    - PyPdfError
- `PyPDF2.pdf` (the `pdf` module) no longer exists. The contents were moved with
  the library. You should most likely import directly from `PyPDF2` instead.
  The `RectangleObject` is in `PyPDF2.generic`.
- The `Resources`, `Scripts`, and `Tests` will no longer be part of the distribution
  files on PyPI. This should have little to no impact on most people. The
  `Tests` are renamed to `tests`, the `Resources` are renamed to `resources`.
  Both are still in the git repository. The `Scripts` are now in
  https://github.com/py-pdf/cpdf. `Sample_Code` was moved to the `docs`.

For a full list of deprecated functions, please see the changelog of version
1.28.0.

New Features (ENH):
-  Improve space setting for text extraction (#922)
-  Allow setting the decryption password in PdfReader.__init__ (#920)
-  Add Page.add_transformation (#883)

Bug Fixes (BUG):
-  Fix error adding transformation to page without /Contents (#908)

Robustness (ROB):
-  Cope with invalid length in streams (#861)

Documentation (DOC):
-  Fix style of 1.25 and 1.27 patch notes (#927)
-  Transformation (#907)

Developer Experience (DEV):
-  Create flake8 config file (#916)
-  Use relative imports (#875)

Maintenance (MAINT):
-  Use Python 3.6 language features (#849)
-  Add wrapper function for PendingDeprecationWarnings (#928)
-  Use new PEP8 compliant names (#884)
-  Explicitly represent transformation matrix (#878)
-  Inline PAGE_RANGE_HELP string (#874)
-  Remove unnecessary generics imports (#873)
-  Remove star imports (#865)
-  merger.py ➔ _merger.py (#864)
-  Type annotations for all functions/methods (#854)
-  Add initial type support with mypy (#853)

Testing (TST):
-  Regression test for xmp_metadata converter (#923)
-  Checkout submodule sample-files for benchmark
-  Add text extracting performance benchmark
-  Use new PyPDF2 API in benchmark (#902)
-  Make test suite fail for uncaught warnings (#892)
-  Remove -OO testrun from CI (#901)
-  Improve tests for convert_to_int (#899)

Full Changelog: 1.28.4...2.0.0
@MartinThoma MartinThoma mentioned this pull request Jun 5, 2022
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.

2 participants