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

Remove star imports #865

Merged
merged 1 commit into from
May 8, 2022
Merged

Remove star imports #865

merged 1 commit into from
May 8, 2022

Conversation

MartinThoma
Copy link
Member

Closes #741

@MartinThoma MartinThoma changed the base branch from main to 2.0.0-dev May 8, 2022 08:48
@codecov
Copy link

codecov bot commented May 8, 2022

Codecov Report

Merging #865 (f41549a) into 2.0.0-dev (9a9cfee) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           2.0.0-dev     #865   +/-   ##
==========================================
  Coverage      82.72%   82.72%           
==========================================
  Files             16       16           
  Lines           3745     3745           
  Branches         794      794           
==========================================
  Hits            3098     3098           
  Misses           472      472           
  Partials         175      175           
Impacted Files Coverage Δ
PyPDF2/_merger.py 67.90% <100.00%> (ø)
PyPDF2/filters.py 75.39% <100.00%> (ø)
PyPDF2/generic.py 86.20% <100.00%> (ø)

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 9a9cfee...f41549a. Read the comment docs.

@MartinThoma MartinThoma merged commit 96711cc into 2.0.0-dev May 8, 2022
@pubpub-zz
Copy link
Collaborator

@MartinThoma
I have a concern about the change introduced : the imports you have corrected are now using absolute name PyPDF2. often for development purpose I'm duplicating the pypdf2 in order to compare the behavior before and after the change. With absolute name this is no more possible. PEP 8 accepting relative path, can you consider the request to stay with relative path?

@MartinThoma
Copy link
Member Author

MartinThoma commented May 11, 2022

Sure! However, I don't understand the problem.

Do you know that you can install PyPDF2 from the local folder via "pip install -e ."? Does that solve the issue?

When a package is installed like this, it is automatically loaded from that location. When you make a change, you don't need to re-install.

@pubpub-zz
Copy link
Collaborator

pubpub-zz commented May 12, 2022

@MartinThoma
My point does not deal with final installation using pip, during development.
This is my scenario:
as a standard I have the PyPDF2 folder which code I use through "import PyPDF2"
very often, when I'm developping/coding some changes, I need to compare results with original code, Thoses changes are written/applied in a copy named PyPDF2b which called through import PyPDF2b.
that way I can easily get some comparison between code behaviors or quickly compare some tracing.
pdf1 = PyPDF2.PdfFileReader(fn)
pdf2 = PyPDF2b.PdfFileReader(fn)

Do you understand my approach?

@MartinThoma
Copy link
Member Author

Ah, interesting. I think I get it and I'll use relative imports again 👍 thanks for helping me understand.

I'm currently sick with fever and cannot concentrate. It will take at least a week until I undo it.

@pubpub-zz
Copy link
Collaborator

Sorry to hear that. Sure it is covid... still in the air
Get better

MartinThoma added a commit that referenced this pull request May 13, 2022
This is important for some PyPDF2 developers development workflows.
It allows them to compare changes more easily.

See #865 (comment)
@MartinThoma
Copy link
Member Author

@pubpub-zz See #875 for the PR. I guess that making everything a relative import is better than just restoring the old state with mixed absolute/relative imports?

@pubpub-zz
Copy link
Collaborator

pubpub-zz commented May 13, 2022

@MartinThoma
I was about to ask what was your preference in order to prepare the PR 😂
Your change looks perfect for me.

Just a last question : there is still some local imports in some functions : do you think we should move them to the top ?
PS : hope you're feeling better

@MartinThoma
Copy link
Member Author

Yes, I feel a lot better today - thanks for asking 🤗 yesterday I had over 39°C fever; compared to that I feel fabulous 😁

Just a last question : there is still some local imports in some functions : do you think we should move them to the top ?

In general, I'd say "yes". The question is if we get circular imports. I think that was the reason for moving the imports into the functions

MartinThoma added a commit that referenced this pull request May 14, 2022
This is important for some PyPDF2 developers development workflows.
It allows them to compare changes more easily.

Additionally, some imports were moved from function-level to module-level.

See #865 (comment)

Co-authored-by: pubpub-zz <4083478+pubpub-zz@users.noreply.github.com>
@MartinThoma MartinThoma deleted the remove-star-imports branch May 26, 2022 08:14
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
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