add type annotations and rework some None/unset/default cases #3201
Conversation
…ermit reloader without public tracebacks
this simply forces the specific test to work. much more work to be done to properly unbreak.
| raise ValueError("Invalid header map behaviour: %s" % val) | ||
|
|
||
|
|
||
| class OnFatal(Setting): |
There was a problem hiding this comment.
Naming pattern clash. Sounds like Server Hook but is not implemented as such.
|
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. |
It would make it easier to maintain projects that depend on this library (I personally don't use an IDE, but we use mypy for very strict type checking throughout all our projects). For example, we don't have anyone on aiohttp at the moment that is familiar with the gunicorn worker, so more type information that can be verified with a type checker would make it easier for us to maintain. I'd also argue that those annotations may actually improve readability. Looking at the diff, most annotations are single word types. Including them means that I can see what types are being used at a glance, whereas right now I'd have to dig through the code to figure that out, or just guess. |
Hence the suggestion of shipping separate .pyi files - for those who care, to be ignored otherwise - instead of placing the annotations inline.
No need to, mypy is perfectly happy to work with
I have been using a version of these for a year now, without any IDE in the loop - purely for CI reasons. I can split out the default settings changes so you can merge the Edit: Split and resubmitted as #3360 |
|
this PR is mixing topic. PLease remove the type hint related change as ai'm not interrested to includes it in gunicorn. The rest looks ok. |
|
let's close this PR and continue discussion on functional changes , which is what really matter. |
|
It would make sense to crate an independent |
Apparently we can ship type annotations in separate
*.pyifiles 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:--config=""does not disable reading configuration. the next best thing, setting--config=/dev/nullloudly complains--reloadhas non-obvious side-effects in error handling, and setting--reload-extra=is silently ignored when set in isolationselfmethod arg.docs/gunicorn_ext.pyto keep documentation unaffectedgid = abs(gid) & 0x7FFFFFFF<= now unnecessary and broken for systems that actually use group id > 2**32 (see gunicorn/util.py set_owner_process Mangling gid on RHEL8 + Google Cloud Platform (very large int group id) #3212)gevent_pywsgiworker class (but this was already noticed independently, see Access log formatting is wrong when using gevent_pywsgi worker class #2940)This is primarily an invitation to discuss potential default settings changes, the attached patches serve as clarification.
Advantages of shipping type annotations:
Disadvantages of shipping type annotations:
Related
Fixes: #2833 #2374 #1393