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

Upgrade pdfminer.six from 20200517 to 20211012 #515

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

jsvine
Copy link
Owner

@jsvine jsvine commented Oct 15, 2021

See pdfminer.six's changelog for details: https://github.com/pdfminer/pdfminer.six/blob/develop/CHANGELOG.md

... but a key difference is an improvement in how it assigns line, rect, and curve objects. (Diagonal two-point lines, for instance, are now line objects instead of curve objects.)

As a result, this commit also adjusts some of the tests, where the pre-20211012 versions had been incorrectly assigning lines as LTCurve objects.

Note: This commit also tweaks CHANGELOG.md to call the next release 0.6.0, since this will be a breaking change for some use-cases — though an improvement, and necessary if we don't want pdfplumber to fall far out of sync with pdfminer.six.

@jsvine jsvine requested a review from samkit-jain October 15, 2021 12:34
See pdfminer.six's changelog for details:
https://github.com/pdfminer/pdfminer.six/blob/develop/CHANGELOG.md

... but a key difference is an improvement in how it assigns `line`,
`rect`, and `curve` objects. (Diagonal two-point lines, for instance,
are now `line` objects instead of `curve` objects.)

As a result, this commit also adjusts some of the tests, where the
pre-20211012 versions had been incorrectly assigning lines as `LTCurve`
objects.
@jsvine jsvine force-pushed the feature/upgrade-pdfminer branch from 1eca63a to 4b61c38 Compare October 15, 2021 12:54
Repository owner deleted a comment from codecov bot Oct 15, 2021
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #515 (5794aaa) into develop (7a65785) will not change coverage.
The diff coverage is n/a.

❗ Current head 5794aaa differs from pull request most recent head 4b61c38. Consider uploading reports for the commit 4b61c38 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #515   +/-   ##
========================================
  Coverage    98.30%   98.30%           
========================================
  Files           10       10           
  Lines         1236     1236           
========================================
  Hits          1215     1215           
  Misses          21       21           

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 7a65785...4b61c38. Read the comment docs.

Copy link
Collaborator

@samkit-jain samkit-jain left a comment

Choose a reason for hiding this comment

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

Long-awaited upgrade. @jsvine Should all the references to the curves property be removed as well if with this uprade, all curves would be lines? For example,

def curves(self):

@jsvine
Copy link
Owner Author

jsvine commented Oct 18, 2021

Good question, and I can see how that'd be confusing. Curve objects do still exist — they're paths with more than 2 points and which do not constitute a rectangle (or multiple consecutive rectangles). The fixes in the new version of pdfminer.six simply assign "LTLine" correctly in instances where it had previously been assigning "LTCurve," for instance diagonal lines. For an example of some paths that are still correctly curve objects, see the shapes on the final page of this PDF: http://www.pdfill.com/example/pdf_drawing_new.pdf

@jsvine jsvine merged commit e1d851a into develop Oct 20, 2021
@jsvine jsvine deleted the feature/upgrade-pdfminer branch October 20, 2021 00:26
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