add type annotations #138
add type annotations #138
Conversation
graingert
force-pushed
the
add-type-annotations
branch
3 times, most recently
from
79982ea to
1d02267
Compare
graingert
force-pushed
the
add-type-annotations
branch
from
5bfed60 to
fb16674
Compare
graingert
force-pushed
the
add-type-annotations
branch
from
c9da8cb to
70a3077
Compare
|
@Kriechi ok finally this is passing, and ready for review :D ! |
| self._maximum_streams + 1 | ||
| ) | ||
| "Refusing to insert %d streams into priority tree at once" | ||
| % (self._maximum_streams + 1) |
There was a problem hiding this comment.
I'd like to switch over to the new F-strings / PEP 498 to improve readability and code flow.
Would you mind including this throughout the project?
There was a problem hiding this comment.
I'll apply pyupgrade --py36-plus, and black, etc in the next PR, I'll copy in the pre-commit config from twisted
| @@ -17,8 +17,11 @@ deps = | |||
| pytest-cov>=2.10,<3 | |||
| pytest-xdist>=2.1,<3 | |||
| hypothesis>=6.9,<6.10 | |||
| extras = | |||
| mypy: mypy | |||
There was a problem hiding this comment.
Any specific reason for extras here? Let's make it and explicit dependency!
There was a problem hiding this comment.
Or alternatively, move mypy testing into a dedicated env, as we have done for wsproto https://github.com/python-hyper/wsproto/blob/55bf7a4ee1e85e3ebc93f04708327b0d0f193925/tox.ini#L40-L46
There was a problem hiding this comment.
It's convenient to use extras when developing outside of tox, I'm planning on moving the test deps to extras too
| @@ -1,3 +1,43 @@ | |||
| [metadata] | |||
There was a problem hiding this comment.
While this doesn't look that bad (actually quite nice), I would prefer to keep the projects within the python-hyper org as similar as possible, especially for all HTTP/2 projects: h2, hpack, hyperframe, and now priority.
Do you have strong arguments or feels for stripping setup.py and moving it to setup.cfg? And what about pyproject.toml, I'd rather not open this topic and stay with the "traditional" way for now.
There was a problem hiding this comment.
setuptools strongly recommends using the declarative metadata for configuration. I'll obviously move everything over to PEP 621 metadata once ready
There was a problem hiding this comment.
It's also easier to move to PEP 621 metadata once everything is in a declarative format, as you can use setuptools' parse_configuration() and toml.dumps
| @@ -5,5 +5,5 @@ graft visualizer | |||
| graft examples | |||
| prune docs/build | |||
| recursive-include *.py | |||
| include README.rst LICENSE CONTRIBUTORS.rst HISTORY.rst tox.ini Makefile | |||
| include README.rst LICENSE CONTRIBUTORS.rst HISTORY.rst tox.ini Makefile src/priority/py.typed | |||
There was a problem hiding this comment.
Shouldn't py.typed already be covered by the graft priority statement above?
But I actually suspect that this should be graft src/priority instead?
There was a problem hiding this comment.
I don't know, but this is how I've seen it done elsewhere
There was a problem hiding this comment.
I'm still trying to fully understand all the moving parts, but from https://packaging.python.org/guides/using-manifest-in/ I would argue that this is not necessary, because we already have py.typed in the package_data.
|
Can you remove |
|
A right, the branch requirement! Done. |
based on and closes python-hyper#138
based on and closes python-hyper#138
based on and closes python-hyper#138
based on and closes python-hyper#138
No description provided.