miguelgrinberg / Flask-SocketIO

Socket.IO integration for Flask applications.
MIT License
5.31k stars 888 forks source link

Add type hints #2010

Closed sohang3112 closed 9 months ago

sohang3112 commented 9 months ago

Add type hints or type stubs so that static analysis tools like mypy can type check code using this library. Then IDEs will also be able to catch any type errors while editing.

miguelgrinberg commented 9 months ago

Feel free to contribute type hints for this package to typeshed. I don't discard that at some point in the future when Python typing is mature enough I will do this myself, but I have too many open source packages and I cannot take on the burden of maintaining type hints for all of them given the effort it currently entails.

sohang3112 commented 9 months ago

@miguelgrinberg I created this pull request in typeshed repository to add type stubs for Flask-SocketIO. Could you please have a look at it and let me know if I have written the wrong type anywhere?

miguelgrinberg commented 9 months ago

I did a very quick review and wrote down some comments. In addition, I personally do not agree with adding types for private classes, attributes and methods, since people shouldn't use them and they are also much more likely to change since I have no obligation to preserve them. But of course, it is your project, so do as you wish. :)

sohang3112 commented 9 months ago

I personally do not agree with adding types for private classes, attributes and methods

I don't think I type hinted any private attributes or methods (please correct me if I did). I did type hint private classes, since the classes were the type of some public arguments. I'm not sure how else to type hint these public arguments.

For example, I type hinted _SocketIOMiddleware because it's the type of this public attribute: sockio_mw: _SocketIOMiddleware | None

miguelgrinberg commented 9 months ago

sockio_mw is not a documented attribute. In fact, none of the attributes of class SocketIO are documented, only the methods are. I don't have an obligation to maintain compatibility for undocumented elements, and for that reason nobody should use them unless they are willing to assume a risk. My opinion is that those should not be exposed in type hints. Check my documentation, anything that is not there should be considered private.

miguelgrinberg commented 9 months ago

Another thing to consider is that you are using the | character liberally in your type hints. If I remember correctly this syntax is only valid for Python 3.10+, but this package supports older Python releases. So maybe it makes more sense to use the more verbose Union and Optional type hints.

sohang3112 commented 9 months ago

Another thing to consider is that you are using the | character liberally in your type hints. If I remember correctly this syntax is only valid for Python 3.10+, but this package supports older Python releases. So maybe it makes more sense to use the more verbose Union and Optional type hints.

@miguelgrinberg Actually the type stubs won't be read by the Python interpreter directly, but rather by static analyzers like mypy, so it's safe to use the latest syntax.

miguelgrinberg commented 9 months ago

You don't seem to understand what I'm saying. If you add types for private elements of the library, users will be more likely to incorporate them in their programs. And these programs will later break when I need to change those internals, which I often do.

sohang3112 commented 9 months ago

@miguelgrinberg As I commented in the PR, I have already removed the private classes and attributes.