Skip to content

Add inline type hints #167

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Add inline type hints #167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7039da1
Add typings
A5rocks Jan 4, 2022
edd97a4
Appease CI
A5rocks Jan 4, 2022
71e1587
Mypy doesn't work on PyPy
A5rocks Jan 4, 2022
fc8f8a3
Shell is not my preferred language
A5rocks Jan 4, 2022
2039b13
Update dependencies
A5rocks Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ dist
examples/fake.*
trio_websocket.egg-info
venv
.mypy_cache/
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ test:

lint:
$(PYTHON) -m pylint trio_websocket/ tests/ autobahn/ examples/
if ! $(PYTHON) --version | grep -q 'PyPy'; then $(PYTHON) -m mypy trio_websocket/; fi

publish:
rm -fr build dist .egg trio_websocket.egg-info
Expand Down
4 changes: 4 additions & 0 deletions mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[mypy]
strict = True
python_version = 3.6
show_error_codes = True
6 changes: 5 additions & 1 deletion pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ disable=duplicate-code,
unused-argument,
unused-variable,
wrong-spelling-in-comment,
wrong-spelling-in-docstring
wrong-spelling-in-docstring,
# needed for explicit re-exports
useless-import-alias,
# false positives with generics
unsubscriptable-object
Comment on lines +25 to +28
Copy link
Member

@belm0 belm0 Jan 10, 2022

Choose a reason for hiding this comment

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

Thank you for the proposal-- I'm a little hesitant to go full type-hints with trio-websocket.

There are benefits, but they doesn't come for free-- at the cost of more verbose code, more complexity, and more dependencies.

Things like the tedious re-exporting, or having to disable useful pylint checkers, are example downsides. Also, it can be corrected, but it seems unnecessary to type hint variables and args that can be inferred by initialization (max_message_size: int = MAX_MESSAGE_SIZE, etc.).

May I ask if you're actively using trio-websocket in a project? Also, did this exercise uncover any bugs in the implementation?

(This comment is overall for the PR, but I'll start the thread here.)

Copy link
Contributor Author

@A5rocks A5rocks Jan 10, 2022

Choose a reason for hiding this comment

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

May I ask if you're actively using trio-websocket in a project?

Yep! Though I have just gotten by by including those stubs in a folder.

Project is open-source actually: https://github.com/A5rocks/bloom

Also, did this exercise uncover any bugs in the implementation?

As mentioned, I started with a base set of stubs so I didn't find any bugs in my implementation.

There are benefits, but they doesn't come for free-- at the cost of more verbose code, more complexity, and more dependencies.

Yep, although I should be able to just extract the stubs out into .pyi files (I haven't played with mypy's stubgen and stubtest yet), if the downsides are too much.

Copy link
Member

@belm0 belm0 Jan 10, 2022

Choose a reason for hiding this comment

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

I don't know if external pyi files improves much. stubgen would just provide a starting point, and would have to be manually edited and kept up to date with the source. And it doesn't address some of the complexity issues (typing can get quite complex, such as nested types, async generators, etc.).

Copy link
Contributor Author

@A5rocks A5rocks Jan 13, 2022

Choose a reason for hiding this comment

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

Hey, sorry for taking a while to get back, wanted to do this when I felt I had enough time to actually write a decent response.

Since you seem to already know many of the downsides, let me try my hand at listing the upsides of having type hints in the code itself:

  1. While not the greatest guarantees, making sure to satisfy type hints can help reduce races (why? because type systems want the state to always be there). This doesn't help too much, but I felt it was worth mentioning.
  2. More up to date than documentation.

One example of this that comes to mind is CloseReason.reason which was documented as a string but was actually optional.

  1. Easier to check that they are right than 3rd party stubs

Here's a relevant part of a diff between stubs extracted from trio-websocket now vs the ones I had included in my project mentioned earlier (left is new stubs, right is old stubs)

90c71
<     def handshake_headers(self) -> typing.Tuple[typing.Tuple[bytes, bytes], ...]: ...
---
>     def handshake_headers(self) -> Tuple[Tuple[str, str], ...]: ...

Also, I had CloseReason.reason as an non-optional string.

  1. Should help other projects.

I'll be honest, I had trouble finding a project using code search that used both mypy and trio-websocket.

However, if I only try to get trio-websocket, then I find some stuff like https://github.com/AcckiyGerman/python_trio_websockets_stress_test

Here's what I get as mypy output:

➜  python_trio_websockets_stress_test git:(master) mypy --check-untyped-defs server.py stress-test.py          (env: tmp-a92fbb44261cfc4) 
Success: no issues found in 2 source files

which isn't great yeah.

If I annotate the function arguments (oops, though check-untyped-defs should have helped a bit maybe)... then it still doesn't find anything. Perhaps this project was too trivial to check against.

So I invite you to try yourself, but I believe this holds true? (If you want to try against some project of yours that has more complex usage then that would be nice)

I'll definitely rebase soon + try to clean up unnecessary type hints, of course.


[REPORTS]
score=no
2 changes: 2 additions & 0 deletions requirements-dev.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ sphinx_rtd_theme
trio>=0.14.0
trustme
twine
mypy; platform.python_implementation == 'CPython'
trio-typing
152 changes: 83 additions & 69 deletions requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,122 +1,130 @@
#
# This file is autogenerated by pip-compile with python 3.6
# This file is autogenerated by pip-compile with python 3.8
# To update, run:
#
# pip-compile --output-file=requirements-dev.txt requirements-dev.in setup.py
#
alabaster==0.7.12
# via sphinx
astroid==2.9.3
astroid==2.12.5
# via pylint
async-generator==1.10
# via
# pytest-trio
# trio
# trio-websocket (setup.py)
attrs==21.4.0
attrs==22.1.0
# via
# -r requirements-dev.in
# outcome
# pytest
# trio
babel==2.9.1
babel==2.10.3
# via sphinx
bleach==4.1.0
bleach==5.0.1
# via readme-renderer
certifi==2021.10.8
build==0.8.0
# via pip-tools
certifi==2022.6.15
# via requests
cffi==1.15.0
cffi==1.15.1
# via cryptography
charset-normalizer==2.0.10
charset-normalizer==2.1.1
# via requests
click==8.0.3
click==8.1.3
# via pip-tools
colorama==0.4.4
# via twine
contextvars==2.4
# via
# sniffio
# trio
coverage[toml]==6.2
commonmark==0.9.1
# via rich
coverage[toml]==6.4.4
# via
# coveralls
# pytest-cov
coveralls==3.3.1
# via -r requirements-dev.in
cryptography==3.3.2
# via trustme
cryptography==37.0.4
# via
# secretstorage
# trustme
dill==0.3.5.1
# via pylint
docopt==0.6.2
# via coveralls
docutils==0.17.1
# via
# readme-renderer
# sphinx
# sphinx-rtd-theme
h11==0.12.0
h11==0.13.0
# via wsproto
idna==3.3
# via
# requests
# trio
# trustme
imagesize==1.3.0
imagesize==1.4.1
# via sphinx
immutables==0.16
# via contextvars
importlib-metadata==4.8.3
importlib-metadata==4.12.0
# via
# click
# keyring
# pep517
# pluggy
# pytest
# sphinx
# twine
iniconfig==1.1.1
# via pytest
isort==5.10.1
# via pylint
jinja2==3.0.3
jeepney==0.8.0
# via
# keyring
# secretstorage
jinja2==3.1.2
# via sphinx
keyring==23.4.1
keyring==23.8.2
# via twine
lazy-object-proxy==1.7.1
# via astroid
markupsafe==2.0.1
markupsafe==2.1.1
# via jinja2
mccabe==0.6.1
mccabe==0.7.0
# via pylint
outcome==1.1.0
mypy==0.971 ; platform_python_implementation == "CPython"
# via -r requirements-dev.in
mypy-extensions==0.4.3
# via
# mypy
# trio-typing
outcome==1.2.0
# via
# pytest-trio
# trio
packaging==21.3
# via
# bleach
# build
# pytest
# sphinx
pep517==0.12.0
# via pip-tools
pip-tools==6.4.0
pep517==0.13.0
# via build
pip-tools==6.8.0
# via -r requirements-dev.in
pkginfo==1.8.2
pkginfo==1.8.3
# via twine
platformdirs==2.4.0
platformdirs==2.5.2
# via pylint
pluggy==1.0.0
# via pytest
py==1.11.0
# via pytest
pycparser==2.21
# via cffi
pygments==2.11.2
pygments==2.13.0
# via
# readme-renderer
# rich
# sphinx
pylint==2.12.2
pylint==2.15.0
# via -r requirements-dev.in
pyparsing==3.0.6
pyparsing==3.0.9
# via packaging
pytest==6.2.5
pytest==7.1.2
# via
# -r requirements-dev.in
# pytest-cov
Expand All @@ -125,20 +133,24 @@ pytest-cov==3.0.0
# via -r requirements-dev.in
pytest-trio==0.7.0
# via -r requirements-dev.in
pytz==2021.3
pytz==2022.2.1
# via babel
readme-renderer==32.0
readme-renderer==37.0
# via twine
requests==2.27.1
requests==2.28.1
# via
# coveralls
# requests-toolbelt
# sphinx
# twine
requests-toolbelt==0.9.1
# via twine
rfc3986==1.5.0
rfc3986==2.0.0
# via twine
rich==12.5.1
# via twine
secretstorage==3.3.3
# via keyring
six==1.16.0
# via bleach
sniffio==1.2.0
Expand All @@ -147,7 +159,7 @@ snowballstemmer==2.2.0
# via sphinx
sortedcontainers==2.4.0
# via trio
sphinx==4.3.2
sphinx==5.1.1
# via
# -r requirements-dev.in
# sphinx-rtd-theme
Expand All @@ -168,47 +180,49 @@ sphinxcontrib-serializinghtml==1.1.5
# via sphinx
sphinxcontrib-trio==1.1.2
# via -r requirements-dev.in
toml==0.10.2
# via
# pylint
# pytest
tomli==1.2.3
tomli==2.0.1
# via
# build
# coverage
# mypy
# pep517
tqdm==4.62.3
# via twine
trio==0.19.0
# pylint
# pytest
tomlkit==0.11.4
# via pylint
trio==0.21.0
# via
# -r requirements-dev.in
# pytest-trio
# trio-typing
# trio-websocket (setup.py)
trio-typing==0.7.0
# via -r requirements-dev.in
trustme==0.9.0
# via -r requirements-dev.in
twine==3.7.1
twine==4.0.1
# via -r requirements-dev.in
typed-ast==1.5.1
# via astroid
typing-extensions==4.0.1
typing-extensions==4.3.0
# via
# astroid
# immutables
# importlib-metadata
# mypy
# pylint
urllib3==1.26.8
# via requests
# rich
# trio-typing
urllib3==1.26.12
# via
# requests
# twine
webencodings==0.5.1
# via bleach
wheel==0.37.1
# via pip-tools
wrapt==1.13.3
wrapt==1.14.1
# via astroid
wsproto==0.15.0
wsproto==1.2.0
# via trio-websocket (setup.py)
zipp==3.6.0
# via
# importlib-metadata
# pep517
zipp==3.8.1
# via importlib-metadata

# The following packages are considered to be unsafe in a requirements file:
# pip
Expand Down
3 changes: 3 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
'Programming Language :: Python :: 3.9',
'Programming Language :: Python :: Implementation :: CPython',
'Programming Language :: Python :: Implementation :: PyPy',
'Typing :: Typed',
],
python_requires=">=3.6",
keywords='websocket client server trio',
Expand All @@ -43,6 +44,8 @@
'trio>=0.11',
'wsproto>=0.14',
],
package_data={'trio_websocket': ['py.typed']},
zip_safe=False,
project_urls={
'Bug Reports': 'https://github.com/HyperionGray/trio-websocket/issues',
'Source': 'https://github.com/HyperionGray/trio-websocket',
Expand Down
36 changes: 18 additions & 18 deletions trio_websocket/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
from ._impl import (
CloseReason,
ConnectionClosed,
ConnectionRejected,
ConnectionTimeout,
connect_websocket,
connect_websocket_url,
DisconnectionTimeout,
Endpoint,
HandshakeError,
open_websocket,
open_websocket_url,
WebSocketConnection,
WebSocketRequest,
WebSocketServer,
wrap_client_stream,
wrap_server_stream,
serve_websocket,
CloseReason as CloseReason,
ConnectionClosed as ConnectionClosed,
ConnectionRejected as ConnectionRejected,
ConnectionTimeout as ConnectionTimeout,
connect_websocket as connect_websocket,
connect_websocket_url as connect_websocket_url,
DisconnectionTimeout as DisconnectionTimeout,
Endpoint as Endpoint,
HandshakeError as HandshakeError,
open_websocket as open_websocket,
open_websocket_url as open_websocket_url,
WebSocketConnection as WebSocketConnection,
WebSocketRequest as WebSocketRequest,
WebSocketServer as WebSocketServer,
wrap_client_stream as wrap_client_stream,
wrap_server_stream as wrap_server_stream,
serve_websocket as serve_websocket,
)
from ._version import __version__
from ._version import __version__ as __version__
Loading