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 Python type annotation #263

Open
simo5 opened this issue Feb 15, 2022 · 13 comments
Open

Add Python type annotation #263

simo5 opened this issue Feb 15, 2022 · 13 comments

Comments

@simo5
Copy link
Member

simo5 commented Feb 15, 2022

type annotations autodocument the function parameters and allow for static analysis and other nice things.
See pythongssapi/python-gssapi#284 for an example.

@embray
Copy link

embray commented Oct 28, 2022

I'd be happy to step up to the plate and work on that inbetween builds of my main project, if the work would be accepted. I have experience with static typing using mypy across some large, non-trivial projects.

@simo5
Copy link
Member Author

simo5 commented Oct 28, 2022

How invasive do you think it will be?
My main concern is if it causes lots of churn or breaks current uses, other than that I would certainly welcome contributions.

@embray
Copy link

embray commented Oct 30, 2022

Since this library does not support Python 2 I think that will help make it a lot easier. As for questions of invasiveness/churn, a lot of it largely depends on the use case desired for adding type checking of which there are mainly two, and are unfortunately at the time largely incompatible (see python/mypy#5028).

The first is to add separate type stubs at least for all public APIs, so that third parties using this library that check statically that they are calling functions correctly. This would be the least invasive because all type annotations would go outside the main code base. This was my original use case.

The second is to type check the implementation, which I think for any security library should be important, since it often can help to shake out subtle implementation bugs or oversights. This unfortunately is more invasive, requiring type annotations in the implementation code. Fortunately, it can be done incrementally, maybe with focus at first on the most critical areas. It can also be ignored as needed, to avoid major refactoring just to satisfy the type checker. AFAICT this library is fairly stable, which helps, and I wouldn't want to go around breaking things just for the sake of satisfying the type checker.

@simo5
Copy link
Member Author

simo5 commented Oct 30, 2022

Not sure I understand the incompatibilities.
Are you saying that first and second use cases cannot both be implemented?

I see both external and internal type checking as useful, but probably the internal one is more useful because there is no guarantee that users will actual type check anything.

I agree that we may want to ignore type checking where the type-checker is going to cause semantic breakage for no good reason.

I like an approach that allows incremental changes and does not require a flag day that changes everything at once.

@embray
Copy link

embray commented Oct 31, 2022

Are you saying that first and second use cases cannot both be implemented?

Both use cases can be implemented because if you add type hints directly in the code then they can of course be used by external users as well.

I just meant that it's not possible to use an external stub file for a module and type check the implementation in that same module.

If you have for example:

  • jwcrypto/jwk.py
  • jwcrypto/jwk.pyi

where the .pyi file is the type stub file, then mypy completely ignores jwk.py and only takes type signatures from the jwk.pyi. That's what this issue is about.

So if you don't mind having type annotations in the main code, and considering that it can be added incrementally, I suggest that would be the way to go.

@simo5
Copy link
Member Author

simo5 commented Oct 31, 2022

I definitely do not want to have to create scripts to maintain two files for each file. So I would certainly go with annotations in the main code.

do you know how annotations interact with sphinx and documentation generation?

@LucasD11
Copy link

How is this going?

It could also be an option to create a separated package jwcrypto-stubs with only .pyi files. and currently I'm maintaining a local typing file in order to use jwcrypto (with .pyi files for jwa, jwk and common).

I'd like to help making changes to this library if you plan to add typing. But if not, I will publish the typing package separately.

@nhairs
Copy link

nhairs commented Dec 1, 2023

Fellow package maintainer here 👋 (ended up on this issue as I was checking if this library had type annotations available). I thought I'd share my thoughts and experiences which may help you.

So I would certainly go with annotations in the main code.

Since you aren't supporting very old versions of python I'd recommend this. It definitely is the easiest in terms of maintenance.

do you know how annotations interact with sphinx and documentation generation?

I'm not familiar with Sphinx, but it appears there is an extension that will read the type annotations when generating docs. This also ensures that you always have the correct types in your docs as you no longer need to edit the doc-string to insert the types. Check the link to see an example of how you'd write your code.

I'm using MkDocs instead of Sphinx, but here's an example of how docs can use the type annotations without needing to specify it in the doc-string

external type checking

Please note that in order to expose typing information to other project you'll need to create jwcrypto/py.typed (empty file). Without this file type checkers will ignore any type annotations in the library.

Aside: when you do this you'll likely want to edit your setup.py classifiers to include Typing :: Typed.

Python Versions

Be aware that the type system is evolving over time including some changes that are not backwards compatible. Based on the current "active" releases, one of the biggest differences is the use of Optional and Union.

As of 3.10 these are both considered deprecated and instead should use | to combine different type annotations. However the use of the pipe character is backward incompatible, so you'll need to either drop support for <3.10 or use the old annotations (and update them in the future). I'd recommend the latter (use old style and update).

In care you're not aware x: Optional[Union[int, float]] = None is equivalent to x: Union[int, float, None] = None, which is equivalent to x: int | float | None = None.

I'm happy to answer any questions you have, but it sounds like there are plenty of other competent people in this thread that can help you out.

@embray
Copy link

embray commented Dec 1, 2023

@nhairs Thanks for your input. I let this slip a long time ago when things got busy in day job. But since we use this package I'm still very much interested in working on gradually adding typing.

On most of my code, at least for libraries, we're still using Optional and Union in order to not lose too much backwards incompatibility. Even if the move is towards deprecating them I think they will be kept around for along time (and can easily be transformed to the new style automatically later).

@Nathan-Furnal
Copy link

@simo5 Is there still an interest for this? I think I could add type hints and modernize some things like use f-strings (added in Python 3.6) if it's something you'd like.

@simo5
Copy link
Member Author

simo5 commented Feb 20, 2024

Yes, I will gladly accept modernization efforts that do not break APIs

@Nathan-Furnal
Copy link

Where can I ask questions about the codebase if I need to? I think there's some places I'll need input about the expected types or usage.

@simo5
Copy link
Member Author

simo5 commented Feb 20, 2024

Feel free to ask them here if you want, or you can email me privately if you prefer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants