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

Basic mypy support #829

Closed
wants to merge 5 commits into from
Closed

Basic mypy support #829

wants to merge 5 commits into from

Conversation

qarmin
Copy link

@qarmin qarmin commented Jun 28, 2023

Part of #827

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 - N/A - this not changes anything

Looks that mypy may really increase usability of this library.
Only part of functions are typed and sometimes info about type is available only in docs, so IDE cannot determine if type is valid or not.

Also this will allow to remove such strange tests - now IDE automatically shows me that this arguments aren't valid

def test_text_badinput():
    pdf = FPDF()
    pdf.add_page()
    pdf.set_font("Times", size=16)
    with pytest.raises(TypeError):
        pdf.text("x", 20, "Hello World")
    with pytest.raises(TypeError):
        pdf.text(20, "y", "Hello World")
    with pytest.raises(AttributeError):
        pdf.text(20, 20, 777)
    with pytest.raises(AttributeError):
        pdf.text(20, 20, (1, 2, 3))
    with pytest.raises(AttributeError):
        pdf.text(20, 20, None)

If someone wants to try implement more types, then I suggest to start with command

mypy fpdf --strict

and if all things are fixed, then removing one by one errors from disable_error_code inside mypy.ini file should be enough to add types to all

I added also swig into CI requirements, because without it I was not able to install all requirements.txt files.

@qarmin qarmin requested a review from Lucas-C as a code owner June 28, 2023 19:44
@Lucas-C
Copy link
Member

Lucas-C commented Jul 1, 2023

Hi @qarmin!

Thank you for devoting some time to this 😊

The GitHub Actions pipeline is currently failing (81 unit tests are KO), so I cannot merge this for now.
It seems that a small regression was introduced in this PR.

I investigated a little by fetching your branch on my computer.
This unit test for example is failing: pytest -vv test/text/test_write.py -k font_stretching
With qpdf installed, I get this diff:

-  b'BT 31.18 807.14 Td (Lorem ipsum Ut nostrud irure) Tj ET',
-  b'BT 31.18 799.14 Td (reprehenderit anim nostrud dolore sed) Tj ET',
+  b'BT 31.18 807.14 Td (Lorem ipsum Ut nostrud irure reprehenderit anim nostrud '
+  b'dolore sed ut) Tj ET',
-  b'BT 31.18 791.14 Td (ut Excepteur dolore ut sunt irure) Tj ET',
?               ^        ---
+  b'BT 31.18 799.14 Td (Excepteur dolore ut sunt irure) Tj ET',
?               ^

This tells me that the line-breaking logic implemented in fpdf2 was somehow affected by your changes.
This will have to be fixed before this PR is merged.

Also this will allow to remove such strange tests - now IDE automatically shows me that this arguments aren't valid

It's good news that, thanks to you additions, some IDEs can perform more checks and alert users of potential errors!
Good job, this would be helpful to many fpdf2 users 👍
However, unit tests do not serve the same purpose as type checking! 😅
We have unit tests to ensure no backward-incompatible changes are made, and no regressions occur, while developping on fpdf2. They are also a useful tool to document how various features are expected to behave.

I added also swig into CI requirements, because without it I was not able to install all requirements.txt files.

That's OK 👍

@@ -169,7 +169,9 @@ def get_width(
"""

if chars is None:
chars = self.characters[start:end]
chars = self.characters[start:end] # type: ignore[assignment]
assert chars is not None # make mypy happy
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.characters being empty should not normally happen, but it might be reasonable to just return 0.0 as the width if it does.

@Avasam
Copy link

Avasam commented Oct 14, 2023

FWIW, I'd recommend running mypy and pyright on the codebase in the CI, at least for the oldest supported Python version, to ensure contributors don't accidentally break the type hints. Invalid syntax and imports should mostly be covered by existing unit tests.

If (and only if) the types are complete, (or at least, more complete than typeshed's) you should also ship a py.typed marker as part of the package. See https://peps.python.org/pep-0561/#packaging-type-information

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.

4 participants