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

fix for #693 #706

Merged
merged 11 commits into from
Feb 23, 2023
Merged

fix for #693 #706

merged 11 commits into from
Feb 23, 2023

Conversation

eroux
Copy link

@eroux eroux commented Feb 22, 2023

fixes #693

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@eroux eroux requested a review from Lucas-C as a code owner February 22, 2023 13:09
@Lucas-C
Copy link
Member

Lucas-C commented Feb 22, 2023

Pylint spotted some minor issues:

pylint fpdf test tutorial/tuto*.py
************* Module fpdf.image_parsing
fpdf/image_parsing.py:390:20: C0209: Formatting a regular string which could be a f-string (consider-using-f-string)
fpdf/image_parsing.py:406:16: C0200: Consider using enumerate instead of iterating with range and len (consider-using-enumerate)
fpdf/image_parsing.py:410:33: C0209: Formatting a regular string which could be a f-string (consider-using-f-string)

CHANGELOG.md Show resolved Hide resolved
fpdf/image_parsing.py Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
eroux and others added 3 commits February 22, 2023 15:44
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
@Lucas-C
Copy link
Member

Lucas-C commented Feb 23, 2023

As described our the development documentation and visible in the GithHub Actions log, you will need to run black on the code to auto-format it:

Run black --check .
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
would reformat /home/runner/work/fpdf2/fpdf2/test/errors/test_FPDF_errors.py

@eroux
Copy link
Author

eroux commented Feb 23, 2023

ah sorry, I only ran it on the fpdf folder, just pushed the reformatted test

fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
fpdf/image_parsing.py Outdated Show resolved Hide resolved
@Lucas-C
Copy link
Member

Lucas-C commented Feb 23, 2023

There are failing unit tests:

FAILED test/test_perfs.py::test_intense_image_rendering - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/fonts/test_add_font.py::test_add_font_uppercase - ZeroDivisionError: division by zero
FAILED test/html/test_html.py::test_html_images - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_html_features - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_img_inside_html_table - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_img_inside_html_table_without_explicit_dimensions - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_img_inside_html_table_centered - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_img_inside_html_table_centered_with_align - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_img_inside_html_table_centered_with_caption - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/html/test_html.py::test_img_not_overlapping - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_full_page_image.py::test_full_width_image - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_full_page_image.py::test_full_height_image - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_image_info.py::test_get_img_info - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_image_info.py::test_get_img_info_data_rgba - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_image_info.py::test_get_img_info_data_palette - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_image_info.py::test_get_img_info_data_gray - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_load_image.py::test_load_base64_data - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_load_image.py::test_load_invalid_base64_data - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_load_image.py::test_share_images_cache - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/test_url_images.py::test_png_url - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/image/png_images/test_png_file.py::test_insert_png_files - AttributeError: 'NoneType' object has no attribute 'read'
FAILED test/layout/test_page_background.py::test_page_background - AttributeError: 'NoneType' object has no attribute 'read'

@eroux
Copy link
Author

eroux commented Feb 23, 2023

you removed the if not img or part of the test, I think that's why the tests are failing. I've restored it in my last commit

@eroux
Copy link
Author

eroux commented Feb 23, 2023

all unit tests pass for me at the latest commit

@Lucas-C
Copy link
Member

Lucas-C commented Feb 23, 2023

Sorry if I broke something in your PR 🤭

There is one last unit test failing:

FAILED test/fonts/test_add_font.py::test_add_font_uppercase - ZeroDivisionError: division by zero

>       scale = 1000 / font["head"].unitsPerEm
E       ZeroDivisionError: division by zero

fpdf/fpdf.py:1800: ZeroDivisionError

@eroux
Copy link
Author

eroux commented Feb 23, 2023

ah yes, I inadvertently pushed a change that git is doing automatically on my machine on test/fonts/Roboto-BoldItalic.TTF but for some reason I can't undo it (git automatically re-does the change)... is there any way you can resotre it to its original state? Otherwise I'll open a new PR

@Lucas-C
Copy link
Member

Lucas-C commented Feb 23, 2023

ah yes, I inadvertently pushed a change that git is doing automatically on my machine on test/fonts/Roboto-BoldItalic.TTF but for some reason I can't undo it (git automatically re-does the change)... is there any way you can resotre it to its original state? Otherwise I'll open a new PR

Maybe the fix could be to simply add the TTF file extension (in uppercase) to that file:
https://github.com/PyFPDF/fpdf2/blob/master/.gitattributes#L8

@eroux
Copy link
Author

eroux commented Feb 23, 2023

it seems to have worked, thanks! I've restored the .TTF file to its previous state (I think), can you give the unit tests a try on your machine?

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Base: 93.77% // Head: 93.66% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (f4b415c) compared to base (907052c).
Patch coverage: 73.33% of modified lines in pull request are covered.

❗ Current head f4b415c differs from pull request most recent head 3d21e48. Consider uploading reports for the commit 3d21e48 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
- Coverage   93.77%   93.66%   -0.12%     
==========================================
  Files          26       26              
  Lines        6829     6868      +39     
  Branches     1216     1226      +10     
==========================================
+ Hits         6404     6433      +29     
- Misses        251      259       +8     
- Partials      174      176       +2     
Impacted Files Coverage Δ
fpdf/image_parsing.py 81.77% <72.72%> (-1.56%) ⬇️
fpdf/fpdf.py 91.65% <100.00%> (-0.01%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lucas-C Lucas-C merged commit 5a2cb13 into py-pdf:master Feb 23, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Feb 23, 2023

Merged!

Thank you for this work on this @eroux 😊
Excellent work!

@Lucas-C Lucas-C mentioned this pull request Feb 23, 2023
5 tasks
@eroux
Copy link
Author

eroux commented Feb 23, 2023

great, thanks! One more for #697 and I'll be able to use fpdf2 for a relatively large project

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.

Feature request: avoid altering images passed to FPDF.image() if there is no need to
2 participants