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

Use PdfReader and PdfMerger #84

Merged
merged 3 commits into from
Dec 28, 2022
Merged

Use PdfReader and PdfMerger #84

merged 3 commits into from
Dec 28, 2022

Conversation

rongxin-liu
Copy link
Contributor

No description provided.

dmalan
dmalan previously approved these changes Dec 28, 2022
Copy link
Member

@dmalan dmalan left a comment

Choose a reason for hiding this comment

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

@rongxin-liu any chance this will speed up merging, which has been slower since our last (necessary) change?

@rongxin-liu
Copy link
Contributor Author

rongxin-liu commented Dec 28, 2022

@rongxin-liu any chance this will speed up merging, which has been slower since our last (necessary) change?

We used to do this for merging PDFs, but WeasyPrint broke at some point (method 1):

    if documents:
        pages = [page for document in documents for page in document.pages]
        documents[0].copy(pages).write_pdf(output)

So we switched to doing this (method 2):

    if documents:
        merger = PdfMerger()
        for document in documents:
            merger.append(PdfReader(io.BytesIO(document.write_pdf())))
        merger.write(output)

I'm unsure how this would impact the speed of merging, but the code owner from WeasyPrint did recommend we adopt method 2 and avoid using method 1 per:
Kozea/WeasyPrint#1662 (comment)

@rongxin-liu
Copy link
Contributor Author

@dmalan I reverted our code to use method 1, since that method is faster and the generated PDF looks fine.

@rongxin-liu rongxin-liu merged commit 98b7cbc into main Dec 28, 2022
@rongxin-liu rongxin-liu deleted the pypdf-updates branch December 28, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants