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

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

Merged
merged 46 commits into from
Aug 3, 2022

Conversation

MartinThoma
Copy link
Member

@MartinThoma MartinThoma commented Aug 1, 2022

Closes #1108
Closes #1117

Co-authored-by: JianzhengLuo [email protected]

JianzhengLuo and others added 30 commits July 15, 2022 20:53
…ebackType` is from `typing` but from `types`.
Co-authored-by: Martin Thoma <[email protected]>
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #1193 (5464bd0) into main (0a6676f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
+ Coverage   92.10%   92.14%   +0.04%     
==========================================
  Files          24       24              
  Lines        4913     4939      +26     
  Branches     1017     1021       +4     
==========================================
+ Hits         4525     4551      +26     
  Misses        244      244              
  Partials      144      144              
Impacted Files Coverage Δ
PyPDF2/_merger.py 94.18% <100.00%> (+0.09%) ⬆️
PyPDF2/_writer.py 88.60% <100.00%> (+0.44%) ⬆️

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...5464bd0. Read the comment docs.

@MartinThoma
Copy link
Member Author

@MasterOdin This PR is just a small refinement after @JianzhengLuo did the hard parts in #1117 . Would you please be so kind to have another look? 🙏

One part that I don't like so far is the "fileobj" parameter name. However, I don't have an idea for a better name. Maybe "stream" to be consistent with other methods?

@MasterOdin
Copy link
Member

The term fileobj is already used in a handful of places in PdfMerger, so I think it might be best to leave it as is to be internally consistent with the class, and then make a separate issue to make PdfMerger consistent with the rest of the codebase which seems to use stream in the way fileobj is used here, and can do a similar trick as #1156 to deprecate the fileobj parameter name (in case anyone uses a named value) in favor of the new name. There's not really a great name to describe this since the type is technically a combination of a path-like object and a file object (where stream is a synonym of this phrase). Perhaps just file, given that it's no longer a builtin in python3 like it was in python2?

"""

@deprecate_bookmark(bookmarks="outline")
def __init__(self, strict: bool = False) -> None:
def __init__(self, strict: bool = False, fileobj: StrByteType = "") -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Should fileobj be allowed to be path-like following from #1190?

Additionally, why not make the type Optional[StrByteType] and just have it default to None instead of ""?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea to allow Path objects 👍

What is the advantage of using None?

if self.fileobj:
print(f"write to {self.fileobj}")
self.write(self.fileobj)
print("nope")
Copy link
Member

Choose a reason for hiding this comment

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

Remove these prints?

Copy link
Member Author

Choose a reason for hiding this comment

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

PyPDF2/_writer.py Show resolved Hide resolved
PyPDF2/_merger.py Outdated Show resolved Hide resolved
tests/test_writer.py Outdated Show resolved Hide resolved
tests/test_merger.py Outdated Show resolved Hide resolved
@MartinThoma MartinThoma merged commit 4aa9ec9 into main Aug 3, 2022
@MartinThoma MartinThoma deleted the JianzhengLuo-add-with-as-usage-#1108 branch August 3, 2022 11:47
@MartinThoma
Copy link
Member Author

@JianzhengLuo Congratulations, your PR got merged 🎉

I've added you to https://pypdf2.readthedocs.io/en/latest/meta/CONTRIBUTORS.html (might need a few minutes to re-build)

I will make a PyPDF2 release to PyPI at the end of the week :-)

@JianzhengLuo
Copy link
Contributor

@JianzhengLuo Congratulations, your PR got merged

I've added you to https://pypdf2.readthedocs.io/en/latest/meta/CONTRIBUTORS.html (might need a few minutes to re-build)

I will make a PyPDF2 release to PyPI at the end of the week :-)

Thanks for finishing the rest! Since I was busy with my math classes these days, I can't reply to the emails, but I am thinking about it and hoping it got merged all day.

There are many difficulties during this PR: Firstly my English isn't very nature and my vocabulary isn't big enough. Secondly, this is my first time to work on GitHub with other contributors, I didn't know too much about some GitHub features, I can't even use Git easily, also I can't code very well, I even hadn't used pytest before, compare to you professors. Thirdly, as you say, there were many other things going on in our lives, the high temperature, for you, and for me, my village couldn't access the Internet for a few days because a truck crashed into an upstream cable. And so on, so on.

But I really felt the kindness went out from other contributors, they, or you, are very patient, and the respect. Especially the respect, although I wrote many bad programs, even made some stupid mistakes, you weren't angry with me, or you did but you didn't speak it out and told a 13-year-old boy how to write it.

I had heard that in some companies like Google, you can feel sufficient respect. I didn't really believe that before, thought it was just some hyperbolical ads. However, compare to another project I am working on (vinceliuice/WhiteSur-icon-theme), by a Chinese designer, I designed two new icons and he just merged it without saying anything even didn't mention me, same as vinceliuice/WhiteSur-gtk-theme, they are also open-sourced projects on GitHub, but they didn't let me feel the respect like this project. I don't think that they meant to do that, but the core of the culture is hard to change. And I believe "sufficient respect" without doubt.

I had learned a lot, big thank you to you, with tearful eyes, really big thank you, quoting a sentence from Zhuge Liang:

临表涕零,不知所言。

@MartinThoma
Copy link
Member Author

You did an excellent job! 👏

Firstly my English isn't very nature and my vocabulary isn't big enough.

Your English is very good, don't worry :-) I'm neither a native speaker. I understand the feeling of not being able to express oneself properly.

Your English is good. Be confident in yourself :-)

my first time to work on GitHub with other contributors

Hehe, I hope it was an overall good experience. I still remember when I made my first contribution (on Scipy; just some documentation). The maintainer was very polite, but also very strict. In the end he asked me to squash+rebase my PR. I completely messed it up in a way that I couldn't recover at the time. But he was gentle and offered me to take care of the rest. I was so happy and proud that I managed to get a PR accepted into SciPy :-)

my village couldn't access the Internet for a few days because a truck crashed into an upstream cable

Hahaha, a classic 😄 I was living in the rural parts of Germany in my childhood and I remember one or two power outages due to construction workers hitting cables 😄

But I really felt the kindness went out from other contributors, they, or you, are very patient, and the respect. Especially the respect, although I wrote many bad programs, even made some stupid mistakes, you weren't angry with me

🤗

a 13-year-old boy

You're only 13? Wow, now I'm impressed! You have an excellent career in front of yourself! Being able to speak English + Chinese + starting so early with professional software development (version control with git, unit testing with pytest, open source development). That is really amazing!

Do you want to become a professional Software Engineer? If you want some hints / ask questions, you can also just sent me an email 🙂

Regarding your comments about respect: I'm trying to build a community around PyPDF2. My motivation here is intrinsic; I simply enjoy writing/having good software + maybe some stories to tell/write how to get there.

I don't earn money with PyPDF2, so I cannot pay people to contribute. That means other peoples motivation has to be something else. It has to be fun to contribute. I think the core parts to make it fun are (1) being gentle/supporting (2) fast feedback (3) consistency + reliability.

(1) + (2) are pretty clear. It helps to always think that others have good intentions / want to help. With this mindset I hope to be nice - and so far I'm super happy with how people react 🎉

(3) is hard. We want to build reliable and well-designed software. That sometimes means to say "no" to a feature. But the important part is to give reasons / to ask others for their opinion.

@JianzhengLuo
Copy link
Contributor

I hope one day I can become a good maintainer, just like you!😊

MartinThoma added a commit that referenced this pull request Aug 7, 2022
New Features (ENH):
-  "with" support for PdfMerger and PdfWriter (#1193)
-  Add AnnotationBuilder.text(...) to build text annotations (#1202)

Bug Fixes (BUG):
-  Allow IndirectObjects as stream filters (#1211)

Documentation (DOC):
-  Font scrambling
-  Page vs Content scaling (#1208)
-  Example for orientation parameter of extract_text (#1206)
-  Fix AnnotationBuilder parameter formatting (#1204)

Developer Experience (DEV):
-  Add flake8-print (#1203)

Maintenance (MAINT):
-  Introduce WrongPasswordError / FileNotDecryptedError / EmptyFileError  (#1201)

Full Changelog: 2.9.0...2.10.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.

Add with ... as ... Usage to PdfMerger()
3 participants