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

add py.typed marker #141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

yagebu
Copy link
Member

@yagebu yagebu commented Jan 2, 2025

No description provided.

@yagebu
Copy link
Member Author

yagebu commented Jan 2, 2025

Build failures are unrelated, see #142 for a PR that fixes them.

Tested locally by installing it, the py.typed marker was installed as well.

@dnicolodi
Copy link
Collaborator

There isn't much value in declaring the package has having typing annotations without verifying that they are complete and correct. It would be counterproductive, actually. Given that the recent work on adding typing to Beancount has created more issue than what it solved, I really don't want to replicate the experience for beangulp users.

@blais
Copy link
Member

blais commented Jan 2, 2025

@dnicolodi Why didn't we modernize all the projects to more recent requirements? 3.10 and above. Gotta let go of the past at some point

@yagebu
Copy link
Member Author

yagebu commented Jan 2, 2025

There isn't much value in declaring the package has having typing annotations without verifying that they are complete and correct. It would be counterproductive, actually.

I disagree. I find it very helpful to have working types for base classes like Importer already even if not all functions and modules are fully typed. I think the incremental approach suggested by mypy (https://mypy.readthedocs.io/en/stable/existing_code.html) works on an ecosystem level as well. Currently, to get type-checking for the bits that are used in Fava I maintain type stubs - those could be removed and all beangulp users could profit from the type annotations.

@yagebu
Copy link
Member Author

yagebu commented Jan 2, 2025

I can also create a PR to run mypy in CI so that the types are checked. There's only a handful of errors

@dnicolodi
Copy link
Collaborator

I think you misunderstood my reply. Typing annotations are useful. Wrong typing annotations are not. A py.typed marker indicates that typing information has at least a good chance to be correct. Because no effort has been put into verifying that typing for beangulp is correct, I'm sure it is not. Thus adding a py.typed marker seems premature. The process should be to verify that typing is correct and then add a py.typed marker. Not the other way around.

If you want to use the existing typing annotations, you can instruct mypy to do it:

[[tool.mypy.overrides]]
module = ["beangulp.*"]
follow_untyped_imports = true

yagebu added 2 commits January 3, 2025 09:07
Without this, mypy will complain when importing Importer from beangulp.
@yagebu
Copy link
Member Author

yagebu commented Jan 3, 2025

See #143 which adds a CI run of mypy to ensure beangulp has a consistent set of type annotations

@yagebu
Copy link
Member Author

yagebu commented Jan 3, 2025

If you want to use the existing typing annotations, you can instruct mypy to do it:

Thanks, TIL - didn't know of this brand-new 1.14 mypy feature yet, that's very useful.

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.

3 participants