gunicorn
gunicorn copied to clipboard
add type annotations and rework some None/unset/default cases
Apparently we can ship type annotations in separate *.pyi
files right now, with no need to await dropping 3.7 compat and no obvious harm to people that have no use for them. Reworking #2377 I ended up with a certainly still imperfect, but already somewhat useful set of annotations. A few default settings in gunicorn popped out as odd:
- setting
--config=""
does not disable reading configuration. the next best thing, setting--config=/dev/null
loudly complains - enabling
--reload
has non-obvious side-effects in error handling, and setting--reload-extra=
is silently ignored when set in isolation - settings that take a callable put the staticmethod in the next line instead of as a decorator. this way static checkers cannot tell the default callables are never meant to receive the usual
self
method arg.- refactoring this requires a minor fix in
docs/gunicorn_ext.py
to keep documentation unaffected
- refactoring this requires a minor fix in
- uid/gid settings default to trying even when not expecting any changes.. is this needed? the only effect appears to be breaking some WSL corner cases (where thats not a no-op but a hard error)
-
gid = abs(gid) & 0x7FFFFFFF
<= now unnecessary and broken for systems that actually use group id > 2**32 (see #3212) - setting --gid=number without setting --uid=number is.. surprising. There is still that root group in the supplementary group list. Looks like Debian did not like this and patched it - though I would probably branch the initgroups case.
- pytest-cov does not mix well with running just coverage run -m pytest (telling me no coverage was collected..) - is it even needed for anything other than invocation brevity, which is already dealt with by inclusion in tox & Makefile?
- There is a str/bytes type confusion in the
gevent_pywsgi
worker class (but this was already noticed independently, see #2940)
This is primarily an invitation to discuss potential default settings changes, the attached patches serve as clarification.
Advantages of shipping type annotations:
- highlight oddities in current behaviour stemming from ambiguous/overloaded methods
- can be checked in CI: automatically receive notification before introducing potential type confusion bugs
- assists in understanding existing code, even where currently little to no documentation exists
- assists in reviewing changes which incorporate type changes, easing complex rebase work of outstanding PRs
- helpful in extracting a list of items to be touch when planning to reclassify previously public/unspecified API into private
- enhances IDE features such as quickly navigating to definitions of types or method arguments
Disadvantages of shipping type annotations:
- people submitting new PRs without updating annotations may see less self-explanatory feedback from CI runners
- as long as any annotation is incorrect or incompatible with a popular tool such as mypy, expect some people to show up and report those bugs in annotations at a time where there already are quite a lot open bugs/PRs
Related
Fixes: #2833 #2374 #1393
Thanks for the patch. However I don't think having type annotation external to the project will be convenient on the long term. This introduces too much constraints as you mention.
Also do we really need to introduce type annotation? What would be the addition there for the project? I think the synttax makes the code barely readable and only adapted for those runing an IDE or vscode. Also I don't want to go to the path we have to set specific rules or properties to ignore some results during type checking. IMO we would better invest in adding property base checking or equivalent there.