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

Introducing new optional parameter at_index to PdfWriter.addpage #4

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

Lucas-C
Copy link

@Lucas-C Lucas-C commented Apr 1, 2021

This is a copy of pmaupin#216

@pep8speaks
Copy link

pep8speaks commented Apr 1, 2021

Hello @Lucas-C! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-01 13:38:39 UTC

@sarnold
Copy link
Owner

sarnold commented Mar 25, 2022

Hi there, I got really busy with RL stuff but I finally did get back to this... Anyway, the static_pdfs repo for tests is now a submodule and the imports in the test files should be changed to from tests import static_pdfs|expected except after I did that, your test fails the hash assert. I don't have a bunch of free time but if you have the time to make the changes in your PRs we can get these merged. Also, please rebase against master first...

@sarnold
Copy link
Owner

sarnold commented Nov 2, 2022

Hmm, after rebase and fixing the imports, this also needed updated hashes for the test to pass. I can go ahead and merge this one and apply the updates for more testing, but you'll need to rebase/update the other 2 PRs and push them again. Thanks!

@sarnold sarnold merged commit 34fa80b into sarnold:master Nov 2, 2022
@sarnold
Copy link
Owner

sarnold commented Nov 2, 2022

Hmm, the hashes in test_add_page appear to be machine/env-specific so for now I'm going to revert my "fix" commit and disable the test for now (in tox.ini). I'm not sure what the proper fix is, and I can't really plumb the depths of PDF files/digests right now.

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.

None yet

3 participants