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

feat: allow parameterize laparams #169

Closed
wants to merge 3 commits into from
Closed

feat: allow parameterize laparams #169

wants to merge 3 commits into from

Conversation

frascuchon
Copy link

This PR resolves issue #168

@jsvine
Copy link
Owner

jsvine commented Jan 13, 2020

Hi @frascuchon, and many thanks for your proposal! I think it’s a good idea to allow for laparams customization. I don’t, however, see where in this PR that’s implemented. Am I missing something?

Part of my difficulty in reading this PR is that there seem to be many code-formatting changes introduced at the same time. While I don’t disagree with most of them, they add a lot of noise to the PR, making it difficult to identify the parts that have meaningfully changed. Can you resubmit the PR without stylistic/formatting changes? (I’d prefer to handle the formatting topic in a separate PR; the approach makes sense to me, but I think it should be consistent throughout the whole project, rather than confined to one file.)

p.s.,. Smart to use the conversion keys as the attributes filter! Great catch.

@jsvine
Copy link
Owner

jsvine commented Jan 13, 2020

Ah, I now see what I was missing: It is already possible to pass custom LAParams to pdfplumber; but when you do, it throws an error (as you note in #168). Your solution is to use the CONVERSIONS dict instead of the IGNORE list as the initial filter in Page.process_object(obj). That does seem to address the error — thanks! — but I want to take a slightly closer look at what was causing the error in the first place. I’ll update this thread when I do.

jsvine added a commit that referenced this pull request Jan 13, 2020
Issue #168 / PR #169. Many thanks to @frascuchon for submitting the PR,
which is the source for the code/idea in this commit.
@jsvine
Copy link
Owner

jsvine commented Jan 13, 2020

Reviewing the cause of that error, it seems relatively benign (the addition of an index property to char objects), and your fix seems like a good one. I have incorporated it in df00787 and credited you. (Thanks!) The fix is now available in v0.5.16.

Closing this PR, but if you want to discuss code-formatting, feel free to open an issue on the topic.

@jsvine jsvine closed this Jan 13, 2020
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