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 static typing #514

Open
Timmmm opened this issue Oct 27, 2023 · 11 comments
Open

Add static typing #514

Timmmm opened this issue Oct 27, 2023 · 11 comments

Comments

@Timmmm
Copy link

Timmmm commented Oct 27, 2023

As far as I can tell this is missing static types or type stubs.

The best way to do it is to add type hints into the code, then you have to add an empty py.typed file in your package (next to __init__.py).

I would recommend using Pyright over Mypy. It's much better. The easiest way to run/enforce it is using the Pyright pre-commit hook.

@JonathonReinhart
Copy link

I think this is a duplicate of #371.

@pmhahn
Copy link
Contributor

pmhahn commented Feb 18, 2025

Yes ,#371 is a duplicate. Can you please close one in the favor of the other?
#533 is also related: If type information is included with pyelftools itself instead of in python/typeshed#11262, that one could also be closed.

I'm asking because I stumbled over missing type annotation for pyelftools myself and would like to add it to pyelftools – I did similar work for other projects in the past.

Before I start working on this I have one question: What is the oldest Python version you're willing to support? Over time may things changed, most importantly if you do from typing import … or from collections.abd import … or which features you can use, e.g. Literal and ParamSpec.
According to https://devguide.python.org/versions/ Python 3.9 is the oldest version still getting security updates. Many other projects use that as a guideline to specify their oldest supported version.

@Timmmm
Copy link
Author

Timmmm commented Feb 18, 2025

@pmhahn I would recommend getting confirmation your PR would be accepted before doing any work here. I have made the mistake of contributing type annotations to Python projects before and then discovering that the authors didn't really get it.

Python 3.9 seems like a reasonable minimum bar to me.

@eliben eliben mentioned this issue Feb 18, 2025
@eliben
Copy link
Owner

eliben commented Feb 18, 2025

I've closed #371

3.9 sounds good

I'm ok with accepting types, but it would be great if this could be done gradually

@sevaa
Copy link
Contributor

sevaa commented Feb 19, 2025

@eliben

This is primarily for the public facing API, right?

@eliben
Copy link
Owner

eliben commented Feb 19, 2025

@eliben

This is primarily for the public facing API, right?

Theoretically, wherever it'd be helpful, I suppose. But the public facing API would be a priority, of course.

@pmhahn
Copy link
Contributor

pmhahn commented Feb 20, 2025

Yes, public API is my preferred thing, but elftools uses construct and things from there become public (by accident).
And while collection information on the types to pass into a function and returned by it, I often have to go down one layer to understand the (internal) functions first, which are called and have to figure out their signature first. When I then have this, it would be a waste to not write it down. (and while doing that, I already found some minor bugs and inconsistencies.)

I just pushed my WIP branch to main...pmhahn:pyelftools:typing – feel free to take a look: the first 9 commits are not related strictly to adding type annotations, but either fix things mypy found for me or cleanup old (Python 2) code. Feel free to fast-path them by cherry-picking those commits.

@pmhahn
Copy link
Contributor

pmhahn commented Feb 20, 2025

How do you want me to handle 🐛 like this in elftools/elf/descriptions.py:250:

def describe_attr_tag_arm(…):
    d_entry =if d_entry is None:
…
        return+ d_entry[val]

Value of type "None" is not indexable

Such things are found by mypy as soon as you add type hints (also to the internal API).

  1. file an issue for each one
  2. fix them directly if I can
  3. add a FIXME # type:ignore[index] comment for now
  4. stop annotating internal APIs 🙈

@Timmmm
Copy link
Author

Timmmm commented Feb 20, 2025

By the way I would really recommend using Pyright over Mypy. It is far better.

In my experience it's best not to try and fix bugs that static types find while you're adding them. I usually add a # TODO. I probably wouldn't ignore the error until you get to the point where the types are actually being checked in CI.

@eliben
Copy link
Owner

eliben commented Feb 21, 2025

How do you want me to handle 🐛 like this in elftools/elf/descriptions.py:250:

def describe_attr_tag_arm(…):
d_entry = …
if d_entry is None:

return … + d_entry[val]

Value of type "None" is not indexable

Such things are found by mypy as soon as you add type hints (also to the internal API).

1. file an issue for each one

2. fix them directly if I can

3. add a `FIXME # type:ignore[index]` comment for now

4. stop annotating internal APIs 🙈

Sending PRs to fix these would be great, as well as cleaning up old Python 2 code. Overall your branch looks OK to me, but I'd really prefer separate PRs for things (not necessarily each bug, but at least related units) to make reviewing easier

@pmhahn
Copy link
Contributor

pmhahn commented Feb 24, 2025

See #598 and #599 and #600 – more after that

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

No branches or pull requests

5 participants