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

addressing issue #578 #581

Merged
merged 3 commits into from
Jan 14, 2022
Merged

addressing issue #578 #581

merged 3 commits into from
Jan 14, 2022

Conversation

augeos-grosso
Copy link
Contributor

@augeos-grosso augeos-grosso commented Jan 13, 2022

This PR fixes #578 .
Indeed the line causing the missed close() call on Windows (and only on Windows it seems) was the one @samkit-jain pointed out.

The original exception was pdfminer.pdfparser.PDFSyntaxError, but i figured it would be best using pdfminer.psparser.PSException, since it is the most general exception i could find that made sense using.

@jsvine jsvine self-requested a review January 13, 2022 15:02
@jsvine jsvine self-assigned this Jan 13, 2022
@jsvine
Copy link
Owner

jsvine commented Jan 13, 2022

Thanks for filing this @augeos-grosso! Two quick things:

  • Could you push an empty/minor commit? It looks like the PR-checking workflow got tripped up on a network issue. We can squash the commit when we merge.

  • Thanks for adding yourself to the contributor's list — well deserved! But could you tweak it (i.e., choose just one link/name/handle) for the purposes of consistency with the other contributors in the list? Doing that would actually take care of the needs-another-commit-to-rerun-workflow situation above.

@jsvine
Copy link
Owner

jsvine commented Jan 13, 2022

Thanks! And, hmmm, workflow still seems gummed up. I'll take a closer look later today; I could just run the tests locally if it comes to that.

@augeos-grosso
Copy link
Contributor Author

Of course!
I'm here to help if you need me

@jsvine jsvine merged commit f7417bc into jsvine:develop Jan 14, 2022
@jsvine
Copy link
Owner

jsvine commented Jan 14, 2022

Thanks again @augeos-grosso! Ran the tests locally — they passed, but coverage decreased since there were no tests added for this change. That's fine. They're simple tests. I've now written them and will push them to develop.

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