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

image() method now inserts .svg images as PDF paths #337

Merged
merged 3 commits into from
Feb 7, 2022
Merged

Conversation

Lucas-C
Copy link
Member

@Lucas-C Lucas-C commented Feb 2, 2022

Could I have your review on this please @torque? 😊

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #337 (484f3ef) into master (b1360a3) will increase coverage by 0.39%.
The diff coverage is 91.01%.

❗ Current head 484f3ef differs from pull request most recent head b983b3e. Consider uploading reports for the commit b983b3e to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   89.99%   90.38%   +0.39%     
==========================================
  Files          19       20       +1     
  Lines        5815     5928     +113     
  Branches     1167     1192      +25     
==========================================
+ Hits         5233     5358     +125     
+ Misses        365      346      -19     
- Partials      217      224       +7     
Impacted Files Coverage Δ
fpdf/svg.py 97.61% <76.00%> (-0.78%) ⬇️
fpdf/fpdf.py 85.35% <87.50%> (+1.04%) ⬆️
fpdf/line_break.py 99.01% <99.01%> (ø)
fpdf/image_parsing.py 91.66% <100.00%> (-2.46%) ⬇️

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 b1360a3...b983b3e. Read the comment docs.

@torque
Copy link

torque commented Feb 5, 2022

I'll take a look at it this weekend.

@torque
Copy link

torque commented Feb 7, 2022

I looked through the changes and they make sense to me.

I do think it would be better if the image() function were to try to detect whether or not the image is an SVG at a later stage (e.g. by looking at the contents see if it starts with <svg and/or parses as XML, though I don't know what sort of false positive or false negative rate this exact check would have), as the current implementation will only work with SVGs provided by filename, while the function's documentation claims it also accepts URLs and BytesIO objects.

@Lucas-C
Copy link
Member Author

Lucas-C commented Feb 7, 2022

Thank you very much for your feedback @torque !
I added a commit to ensure that a <svg> can be passed as BytesIO to FPDF.image

@Lucas-C Lucas-C merged commit ba9d99e into master Feb 7, 2022
@Lucas-C Lucas-C deleted the vector_image branch February 7, 2022 09:16
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