Add `with ... as ...` usage #1117

JianzhengLuo · 2022-07-15T12:59:12Z

Check the code, please?
By the way, would you like to add me into the Contributors of this project?

PyPDF2/_merger.py

PyPDF2/_writer.py

Co-authored-by: Matthew Peveler <[email protected]>

…ebackType` is from `typing` but from `types`.

JianzhengLuo · 2022-07-17T00:43:40Z

Already modified according to @MasterOdin requested changes, but why can't this request merge?

MasterOdin

If you click "Details" for any of the failed test runs, you will see the error that's happening:

PyPDF2/_writer.py:169: error: Function is missing a type annotation  [no-untyped-def]
PyPDF2/_writer.py:173: error: Function is missing a return type annotation  [no-untyped-def]
PyPDF2/_writer.py:821: error: Argument 1 to "write_stream" of "PdfWriter" has incompatible type "Union[str, BytesIO, BufferedReader, BufferedWriter, FileIO]"; expected "Union[BytesIO, BufferedReader, BufferedWriter, FileIO]"  [arg-type]
PyPDF2/_writer.py:824: error: Item "str" of "Union[str, BytesIO, BufferedReader, BufferedWriter, FileIO]" has no attribute "close"  [union-attr]
PyPDF2/_merger.py:101: error: Function is missing a return type annotation  [no-untyped-def]

PyPDF2/_writer.py

PyPDF2/_merger.py

MartinThoma · 2022-07-17T19:56:23Z

By the way, would you like to add me into the Contributors of this project?

We currently don't have a dedicated contributors page / document. I was thinking about adding it though :-) #798

MartinThoma · 2022-07-17T20:01:11Z

Already modified according to @MasterOdin requested changes, but why can't this request merge?

Currently several tests fail because they are missing type annotations:

PyPDF2/_writer.py:169: error: Function is missing a type annotation  [no-untyped-def]
PyPDF2/_writer.py:173: error: Function is missing a return type annotation  [no-untyped-def]
PyPDF2/_writer.py:821: error: Argument 1 to "write_stream" of "PdfWriter" has incompatible type "Union[str, BytesIO, BufferedReader, BufferedWriter, FileIO]"; expected "Union[BytesIO, BufferedReader, BufferedWriter, FileIO]"  [arg-type]
PyPDF2/_writer.py:824: error: Item "str" of "Union[str, BytesIO, BufferedReader, BufferedWriter, FileIO]" has no attribute "close"  [union-attr]
PyPDF2/_merger.py:101: error: Function is missing a return type annotation  [no-untyped-def]

@MasterOdin has already added change requests that would fix those. You just need to merge them :-)

Co-authored-by: Matthew Peveler <[email protected]>

…gLuo/PyPDF2 into add-with-as-usage-#1108

codecov · 2022-07-18T01:15:49Z

Codecov Report

Merging #1117 (336053a) into main (0a6676f) will decrease coverage by 0.27%.
The diff coverage is 75.00%.

❗ Current head 336053a differs from pull request most recent head 092f209. Consider uploading reports for the commit 092f209 to get more accurate results

@@            Coverage Diff             @@
##             main    #1117      +/-   ##
==========================================
- Coverage   92.10%   91.82%   -0.28%     
==========================================
  Files          24       24              
  Lines        4913     4699     -214     
  Branches     1017      968      -49     
==========================================
- Hits         4525     4315     -210     
+ Misses        244      237       -7     
- Partials      144      147       +3

Impacted Files	Coverage Δ
PyPDF2/_merger.py	`91.32% <66.66%> (-2.78%)`	⬇️
PyPDF2/_writer.py	`88.34% <78.94%> (+0.17%)`	⬆️
PyPDF2/filters.py	`92.52% <0.00%> (-2.81%)`	⬇️
PyPDF2/_encryption.py	`85.08% <0.00%> (-0.91%)`	⬇️
PyPDF2/_cmap.py	`93.81% <0.00%> (-0.45%)`	⬇️
PyPDF2/_page.py	`92.60% <0.00%> (-0.02%)`	⬇️
PyPDF2/xmp.py	`89.16% <0.00%> (ø)`
PyPDF2/types.py	`100.00% <0.00%> (ø)`
PyPDF2/constants.py	`100.00% <0.00%> (ø)`
... and 4 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 0a6676f...092f209. Read the comment docs.

JianzhengLuo · 2022-07-18T01:30:05Z

What's the problem again?
75.00% of diff hit (target 91.97%)?

JianzhengLuo · 2022-07-20T10:55:05Z

@MartinThoma @MasterOdin Are you busy these days? I requested a review two days ago, but I didn't get a response till now, what happened?

MartinThoma · 2022-07-20T21:16:32Z

@JianzhengLuo Please be patient - we probably all have full-time jobs and other things going on in our lives. For me, I say that there is currently a heat wave in Germany which means my appartment is at 32°C and more all the time - that makes pretty much anything where I need to think really hard.

PyPDF2/_writer.py

MartinThoma

Could you please add a unit test? If you need help with that, just let me know :-)

JianzhengLuo · 2022-07-21T00:25:22Z

@JianzhengLuo Please be patient - we probably all have full-time jobs and other things going on in our lives. For me, I say that there is currently a heat wave in Germany which means my appartment is at 32°C and more all the time - that makes pretty much anything where I need to think really hard.

I'm sorry for making you angry, I knew that clearly - it's a volunteer job. You know, language is interesting, I just wanted to see what can I do for you, since adults are much busier than children. But when you read this, you might feel angry that I was not patiently and not considerate of you. Maybe next time I can add a emoji so the atmosphere will be more relaxing.

JianzhengLuo · 2022-07-21T12:29:17Z

Hmm... The problems caused by pytest still exist. What can I do?

JianzhengLuo · 2022-07-21T12:31:47Z

Some test_encryption errors also raised on my computer, but I didn't change anything about it. That's wired, could you help me find out what caused this, please? @MartinThoma

MartinThoma · 2022-07-22T05:46:59Z

The encryption issue is weird, but let's first focus on the problem where I know how to fix it:

Please restore test_merge
Please crate a merger_operate as below:

This one should fix several issues:

def merger_operate(merger):
    pdf_path = os.path.join(RESOURCE_ROOT, "crazyones.pdf")
    outline = os.path.join(RESOURCE_ROOT, "pdflatex-outline.pdf")

    merger.append(pdf_path)
    merger.append(outline)

PyPDF2/_writer.py

JianzhengLuo · 2022-07-22T05:59:33Z

The encryption issue is weird, but let's first focus on the problem where I know how to fix it:

Please restore test_merge

Please crate a merger_operate as below:

This one should fix several issues:
def merger_operate(merger):
    pdf_path = os.path.join(RESOURCE_ROOT, "crazyones.pdf")
    outline = os.path.join(RESOURCE_ROOT, "pdflatex-outline.pdf")

    merger.append(pdf_path)
    merger.append(outline)

In that way, how could we test the other features in the original code? Do you mean we do this first to see where the problem is?

After I did what you said, and removed the uncorrelated test files such as test_encryption.py, I got the result below:

FAILED tests/test_merger.py::test_trim_outline_list - PyPDF2.errors.PdfReadError: Cannot read an empty file
FAILED tests/test_merger.py::test_sweep_recursion2[https://corpora.tika.apache.org/base/docs/govdocs1/924/924794.pdf-tika-924794.pdf] - PyPDF2.errors.PdfReadError: Cannot read an empty file
FAILED tests/test_writer.py::test_encrypt[True] - SystemError: more argument specifiers than keyword list entries (remaining format:'s#')
FAILED tests/test_writer.py::test_encrypt[False] - SystemError: more argument specifiers than keyword list entries (remaining format:'s#')

And then? @MartinThoma

By the way, why can't I re-request review to you?

Co-authored-by: Martin Thoma <[email protected]>

… workflow Co-authored-by: Martin Thoma <[email protected]>

…tream()`

PyPDF2/_merger.py

… the existing workflow Co-authored-by: Matthew Peveler <[email protected]>

MartinThoma

Could you please take care of those issues?

./PyPDF2/_writer.py:820:12: F821 undefined name 'fileobj'
./PyPDF2/_writer.py:821:48: F821 undefined name 'fileobj'
./PyPDF2/_writer.py:823:23: F821 undefined name 'fileobj'
./PyPDF2/_writer.py:824:30: F821 undefined name 'fileobj'

JianzhengLuo · 2022-07-24T07:34:57Z

Could you please take care of those issues?

./PyPDF2/_writer.py:820:12: F821 undefined name 'fileobj'
./PyPDF2/_writer.py:821:48: F821 undefined name 'fileobj'
./PyPDF2/_writer.py:823:23: F821 undefined name 'fileobj'
./PyPDF2/_writer.py:824:30: F821 undefined name 'fileobj'

Very, very sorry for that! That wasn't an error before the commit a7346ef. I will fix that immediately.

MartinThoma · 2022-07-24T07:38:04Z

No problem - don't worry :-)

MartinThoma · 2022-07-24T07:39:30Z

I'm happy that you're working on this improvement - there isn't much missing to finish it.

I realize that it becomes frustrating when the PR is not merged for so long. If you want, I can finish it. Just let me know :-)

JianzhengLuo · 2022-07-24T07:45:42Z

I'm happy that you're working on this improvement - there isn't much missing to finish it.

I realize that it becomes frustrating when the PR is not merged for so long. If you want, I can finish it. Just let me know :-)

It seems that I am annoying you, sorry for that! As the errors are too wired, since I didn't even change anything, than finish it if you like! Thank you.

MartinThoma · 2022-07-24T07:48:11Z

It seems that I am annoying you, sorry for that!

No - everything is good 🤗 Don't worry :-)

MartinThoma · 2022-08-02T11:16:56Z

@JianzhengLuo Your PR (now #1193 ) will be merged this week :-) Thank you for all the effort you put into it 🤗

I've added you to the CONTRIBUTORS.md file - if you want another text / a link to some profile, just let me know

Closes #1108 Closes #1117 Full credit for this PR goes to JianzhengLuo Co-authored-by: JianzhengLuo <[email protected]>

Add with ... as ... usage (#1108)

a2b7b55

MasterOdin requested changes Jul 15, 2022

View reviewed changes

JianzhengLuo and others added 8 commits July 16, 2022 07:51

Update PyPDF2/_merger.py according to @MasterOdin's suggestions

2a88bd5

Co-authored-by: Matthew Peveler <[email protected]>

Update PyPDF2/_merger.py according to @MasterOdin's suggestions

b6d0351

Update PyPDF2/_merger.py according to @MasterOdin's suggestions

80813aa

Update PyPDF2/_merger.py according to @MasterOdin's suggestions

e6ec1f6

Update PyPDF2/_merger.py according to @MasterOdin's suggestions

386be5b

Sorry, I forgot to run before committing, so didn't notice that `Trac…

31786bb

…ebackType` is from `typing` but from `types`.

Modify the wrong closing place that cause.

ae1bec0

Merge branch 'main' into add-with-as-usage-#1108

0b7c64e

JianzhengLuo requested a review from MasterOdin July 17, 2022 00:37

MasterOdin requested changes Jul 17, 2022

View reviewed changes

PyPDF2/_writer.py Outdated Show resolved Hide resolved

PyPDF2/_writer.py Outdated Show resolved Hide resolved

PyPDF2/_writer.py Outdated Show resolved Hide resolved

PyPDF2/_writer.py Outdated Show resolved Hide resolved

PyPDF2/_merger.py Outdated Show resolved Hide resolved

JianzhengLuo and others added 6 commits July 18, 2022 08:30

Modify PyPDF2/_writer.py according to @MasterOdin's suggestions

b695113

Co-authored-by: Matthew Peveler <[email protected]>

Modify PyPDF2/_writer.py according to @MasterOdin's suggestions

58797c2

Co-authored-by: Matthew Peveler <[email protected]>

Modify PyPDF2/_writer.py according to @MasterOdin's suggestions

1caa9ec

Co-authored-by: Matthew Peveler <[email protected]>

Modify PyPDF2/_writer.py according to @MasterOdin's suggestions

4fbe3cc

Co-authored-by: Matthew Peveler <[email protected]>

Modify PyPDF2/_writer.py according to @MasterOdin's suggestions

d598e8a

Merge branch 'add-with-as-usage-#1108' of https://github.com/Jianzhen…

7ecf9ff

…gLuo/PyPDF2 into add-with-as-usage-#1108

JianzhengLuo requested a review from MasterOdin July 18, 2022 01:30

Merge branch 'main' into add-with-as-usage-#1108

562ebc7

MartinThoma reviewed Jul 20, 2022

View reviewed changes

PyPDF2/_writer.py Outdated Show resolved Hide resolved

MartinThoma requested changes Jul 20, 2022

View reviewed changes

Fixed wrong test function names

cda38ac

Fixed those I can fix only.

0b18198

MartinThoma reviewed Jul 22, 2022

View reviewed changes

PyPDF2/_writer.py Outdated Show resolved Hide resolved

MartinThoma reviewed Jul 22, 2022

View reviewed changes

PyPDF2/_writer.py Outdated Show resolved Hide resolved

JianzhengLuo and others added 3 commits July 22, 2022 19:29

Removed useless else section

62f970f

Co-authored-by: Martin Thoma <[email protected]>

Renamed argument name fileobj back to stream to keep the existing…

d85c91b

… workflow Co-authored-by: Martin Thoma <[email protected]>

Modified annoation for PdfWriter().write() and `PdfWriter().write_s…

5cd3f3e

…tream()`

MasterOdin reviewed Jul 22, 2022

View reviewed changes

PyPDF2/_merger.py Outdated Show resolved Hide resolved

JianzhengLuo and others added 2 commits July 23, 2022 15:04

Switched fileobj and strict in initializing PdfWriter() to keep…

a7346ef

… the existing workflow Co-authored-by: Matthew Peveler <[email protected]>

Modified annoation to make the it match the arguments

d3e62ee

JianzhengLuo requested review from MasterOdin and MartinThoma and removed request for MartinThoma July 23, 2022 07:07

MartinThoma requested changes Jul 24, 2022

View reviewed changes

Modified undefined name fileobj

59be94d

Merge branch 'main' into add-with-as-usage-#1108

092f209

MartinThoma mentioned this pull request Aug 1, 2022

ENH: "with" support for PdfMerger and PdfWriter #1193

Merged

MartinThoma changed the title ~~Add with ... as ... usage (#1108)~~ Add with ... as ... usage Aug 1, 2022

JianzhengLuo closed this Aug 2, 2022

MartinThoma added a commit that referenced this pull request Aug 3, 2022

ENH: "with" support for PdfMerger and PdfWriter (#1193)

4aa9ec9

Closes #1108 Closes #1117 Full credit for this PR goes to JianzhengLuo Co-authored-by: JianzhengLuo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `with ... as ...` usage #1117

Add `with ... as ...` usage #1117

JianzhengLuo commented Jul 15, 2022

JianzhengLuo commented Jul 17, 2022

MasterOdin left a comment

MartinThoma commented Jul 17, 2022

MartinThoma commented Jul 17, 2022

codecov bot commented Jul 18, 2022 •

edited

Loading

JianzhengLuo commented Jul 18, 2022

JianzhengLuo commented Jul 20, 2022

MartinThoma commented Jul 20, 2022

MartinThoma left a comment

JianzhengLuo commented Jul 21, 2022

JianzhengLuo commented Jul 21, 2022

JianzhengLuo commented Jul 21, 2022 •

edited

Loading

MartinThoma commented Jul 22, 2022

JianzhengLuo commented Jul 22, 2022 •

edited

Loading

MartinThoma left a comment

JianzhengLuo commented Jul 24, 2022

MartinThoma commented Jul 24, 2022

MartinThoma commented Jul 24, 2022

JianzhengLuo commented Jul 24, 2022

MartinThoma commented Jul 24, 2022

MartinThoma commented Aug 2, 2022

Add with ... as ... usage #1117

Add with ... as ... usage #1117

Conversation

JianzhengLuo commented Jul 15, 2022

JianzhengLuo commented Jul 17, 2022

MasterOdin left a comment

Choose a reason for hiding this comment

MartinThoma commented Jul 17, 2022

MartinThoma commented Jul 17, 2022

codecov bot commented Jul 18, 2022 • edited Loading

Codecov Report

JianzhengLuo commented Jul 18, 2022

JianzhengLuo commented Jul 20, 2022

MartinThoma commented Jul 20, 2022

MartinThoma left a comment

Choose a reason for hiding this comment

JianzhengLuo commented Jul 21, 2022

JianzhengLuo commented Jul 21, 2022

JianzhengLuo commented Jul 21, 2022 • edited Loading

MartinThoma commented Jul 22, 2022

JianzhengLuo commented Jul 22, 2022 • edited Loading

MartinThoma left a comment

Choose a reason for hiding this comment

JianzhengLuo commented Jul 24, 2022

MartinThoma commented Jul 24, 2022

MartinThoma commented Jul 24, 2022

JianzhengLuo commented Jul 24, 2022

MartinThoma commented Jul 24, 2022

MartinThoma commented Aug 2, 2022

Add `with ... as ...` usage #1117

Add `with ... as ...` usage #1117

codecov bot commented Jul 18, 2022 •

edited

Loading

JianzhengLuo commented Jul 21, 2022 •

edited

Loading

JianzhengLuo commented Jul 22, 2022 •

edited

Loading