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

Specify a minimal version of fontTools in setup.py #538

Merged
merged 25 commits into from
Sep 20, 2022

Conversation

TchilDill
Copy link

@TchilDill TchilDill commented Sep 14, 2022

This PR aims to fix #524

It includes:

  • Changes in setup.py and specify a fixed version of fontTools (latest tested: v4.37.1).
  • Changes in fpdf.py to generate a warning if the fontTools package's version is something else than the above.
  • Changes in CHANGELOG.md with above updates
  • Changes from Black to .py files for proper formatting.

Checklist:

  • 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 - N/A

  • 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 - N/A

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

I have 5 failing tests with pytest, however they are not affected by my changes. Please see below.

=========================== short test summary info ===========================
FAILED test/image/image_types/test_insert_images.py::test_insert_jpg - Assert...
FAILED test/image/image_types/test_insert_images.py::test_insert_jpg_flatedecode
FAILED test/image/image_types/test_insert_images.py::test_insert_png_alpha_dctdecode
FAILED test/image/png_indexed/test_png_indexed.py::test_png_indexed_no_transparency
FAILED test/layout/test_page_background.py::test_page_background - AssertionE...
======= 5 failed, 968 passed, 2 skipped, 2 warnings in 76.42s (0:01:16) =======

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

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!

I'm going to approve the GitHub Actions pipeline execution,
but there should not be that many changes triggered by black,
as we already ensure the code formatting in the CI pipeline:
https://github.com/PyFPDF/fpdf2/blob/master/.github/workflows/continuous-integration-workflow.yml#L38

Is you version of black up to date?
What caused all those code style changes in the PR?

fpdf/fpdf.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Lucas-C Lucas-C changed the title Issue 524 Specify a mnimal version of fontTools in setup.py Sep 14, 2022
@Lucas-C Lucas-C changed the title Specify a mnimal version of fontTools in setup.py Specify a minimal version of fontTools in setup.py Sep 14, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Sep 14, 2022

I have 5 failing tests with pytest, however they are not affected by my changes. Please see below.

=========================== short test summary info ===========================
FAILED test/image/image_types/test_insert_images.py::test_insert_jpg - Assert...
FAILED test/image/image_types/test_insert_images.py::test_insert_jpg_flatedecode
FAILED test/image/image_types/test_insert_images.py::test_insert_png_alpha_dctdecode
FAILED test/image/png_indexed/test_png_indexed.py::test_png_indexed_no_transparency
FAILED test/layout/test_page_background.py::test_page_background - AssertionE...
======= 5 failed, 968 passed, 2 skipped, 2 warnings in 76.42s (0:01:16) =======

Sadly the logs your provided are truncated and does not allow me to understand why the tests are failing in your development environment. However if you only make a change to setup.py, you won't need to run all the unit tests "locally". The GitHub Actions pipeline will be enough to ensure that no regression happens.

CHANGELOG.md Show resolved Hide resolved
fpdf/fpdf.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 15, 2022

Codecov Report

Merging #538 (54f8baf) into master (943e397) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
- Coverage   94.10%   93.98%   -0.12%     
==========================================
  Files          22       22              
  Lines        6206     6087     -119     
  Branches     1265     1235      -30     
==========================================
- Hits         5840     5721     -119     
  Misses        191      191              
  Partials      175      175              
Impacted Files Coverage Δ
fpdf/svg.py 95.94% <100.00%> (+0.02%) ⬆️
fpdf/prefs.py 90.90% <0.00%> (-1.95%) ⬇️
fpdf/drawing.py 92.73% <0.00%> (-0.28%) ⬇️
fpdf/syntax.py 95.37% <0.00%> (-0.05%) ⬇️
fpdf/sign.py 100.00% <0.00%> (ø)
fpdf/enums.py 100.00% <0.00%> (ø)
fpdf/__init__.py 100.00% <0.00%> (ø)
fpdf/html.py 96.55% <0.00%> (+0.02%) ⬆️
fpdf/fpdf.py 92.59% <0.00%> (+0.02%) ⬆️

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

@TchilDill
Copy link
Author

TchilDill commented Sep 15, 2022

when rebasing the PR, I had to commit changes from an example csv file as it was seen to have "changed".

The content of the file seem to be exactly the same though. I couldn't figure out the reason it found this file as changed.

How can I take this file away from the PR?

@Lucas-C
Copy link
Member

Lucas-C commented Sep 15, 2022

when rebasing the PR, I had to commit changes from an example csv file as it was seen to have "changed".
The content of the file seem to be exactly the same though. I couldn't figure out the reason it found this file as changed.

It's a matter of line endings (CRLF vs LF).
I fixed that on the master branch of this repo, rebasing should solve this issue

Lucas-C and others added 7 commits September 15, 2022 12:03
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
@gmischler
Copy link
Collaborator

Sorry, I'm outbidding you on the minimum version of fonttools in #545, so this PR is probably obsolete now... 😉

@Lucas-C
Copy link
Member

Lucas-C commented Sep 19, 2022

Sorry, I'm outbidding you on the minimum version of fonttools in #545, so this PR is probably obsolete now... 😉

I'd prefer to merge @guillaume-dotcom contribution first, given the time he put in this PR.
The branch just needs to be rebased.
I'll wait until the end of the week and only then I'll consider merging PR #545 first

@gmischler
Copy link
Collaborator

I'd prefer to merge @guillaume-dotcom contribution first, given the time he put in this PR.

@Lucas-C, Fine with me, just go ahead if it's ready.

@TchilDill
Copy link
Author

I'd prefer to merge @guillaume-dotcom contribution first, given the time he put in this PR.
The branch just needs to be rebased.
I'll wait until the end of the week and only then I'll consider merging PR #545 first

@Lucas-C thanks for this :). Appreciated.
I have now rebased the branch to master of this repo.

@Lucas-C Lucas-C merged commit af12339 into py-pdf:master Sep 20, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Sep 20, 2022

Merged!
Thank you @guillaume-dotcom 😊

@TchilDill TchilDill deleted the issue-524 branch September 20, 2022 12:43
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.

.add_font() -- AttributeError: 'table__n_a_m_e' object has no attribute 'getBestFullName'
3 participants