| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
Documentation build overview11 files changed · ± 11 modified ± Modified |
Sorry, something went wrong.
|
The purpose was to document those attributes, not suppress them. Please do so. |
Sorry, something went wrong.
There was a problem hiding this comment.
Please document the header objects properly.
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. And if you don't make the requested changes, you will be put in the comfy chair! |
Sorry, something went wrong.
Replace the "!"-suppressed references with real targets: .. method:: directives for Headers.keys(), Headers.values() and Headers.items(), and .. attribute:: directives for WSGIServer.base_environ, BaseHandler.environ and SimpleHandler's stdin, stdout and stderr. Correct the SimpleHandler paragraph while documenting it: the constructor stores the supplied environment in base_env, not environ, and add_cgi_vars() merges it into BaseHandler.environ during setup_environ(). paste.lint (a third-party module) and FileWrapper.close (bound conditionally from the wrapped object, never defined on the class) keep the "!" prefix, since neither can have a real target.
|
Thanks, that's fair. I've replaced the suppressions with real targets:
One thing I ran into while doing that: the SimpleHandler paragraph was inaccurate. It said the supplied environment is stored in an environ attribute, but __init__ stores it in base_env, and add_cgi_vars() merges that into BaseHandler.environ during setup_environ(). Rather than document a SimpleHandler.environ that does not hold what the sentence claimed, I reworded the sentence to match the code. Happy to split that into its own PR if you would rather keep this one purely markup. Two references are still suppressed, and I believe that is correct in both cases:
One more that I left alone deliberately, so tell me if you want it in scope: the class description still points get and setdefault at dict.get and dict.setdefault, but Headers defines its own, with case-insensitive lookup and multi-valued-header semantics that the dict methods do not have. That text predates this PR so I did not touch it, but it is the same kind of fix if you want it here. I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
If possible add versionadded/versionchanged directives to methods/atters that were not present at the beginning (i.e. if they were added later).
And also, if you using an agent, make sure that you review its output. Agents are quite bad at placement in my experience.
Sorry, something went wrong.
| ``len()`` of a :class:`Headers` object is the same as the length of its | ||
| :meth:`items`, which is the same as the length of the wrapped header list. In | ||
| fact, the :meth:`items` method just returns a copy of the wrapped header list. | ||
| :meth:`items`, which is the same as the length of the wrapped header list. |
There was a problem hiding this comment.
The "in fact" got dropped
Sorry, something went wrong.
| :meth:`items`, which is the same as the length of the wrapped header list. | ||
|
|
||
|
|
||
| .. method:: Headers.keys() |
There was a problem hiding this comment.
Above we say that Headers implement dict.get. So we could maybe reference dict.keys() instead. I do not remember whether it is implemented specifically or not. If .keys() etc are explicit methods on the Header class, you can keep this.
Sorry, something went wrong.
| :meth:`get_app` exists mainly for the benefit of request handler instances. | ||
|
|
||
|
|
||
| .. attribute:: WSGIServer.base_environ |
There was a problem hiding this comment.
Check if this page documents attributes before methods or vice-versa first. I believe attributes are put first. Then move that one accordingly. It is weird to have this attribute documentation here
Sorry, something went wrong.
| and the :attr:`server_software` attribute is set. | ||
|
|
||
|
|
||
| .. attribute:: BaseHandler.environ |
There was a problem hiding this comment.
Move this in the attributes section
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
Closing since changes were not made since then. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes the 18 nit-picky Sphinx reference warnings in Doc/library/wsgiref.rst and removes the file from Doc/tools/.nitignore.
Where a correct target already exists, the reference is qualified so it resolves:
The remaining names have no documented target anywhere: Headers.keys, Headers.values, Headers.items, WSGIServer.base_environ, BaseHandler.environ and SimpleHandler.stdin / stdout / stderr are described only in prose, and paste.lint is third party. Those use the ! prefix, which keeps the semantic markup and drops the link.
I kept this to reference fixes so the diff stays limited to removing the warnings. If you would prefer the undocumented Headers methods and handler attributes to become proper .. method:: and .. attribute:: entries instead (as was done for the lzma constants in gh-151949), I am happy to do that here or in a follow-up.
Verified with a fresh nit-picky build: warnings for this file go from 18 to 0, total build warnings go from 1352 to 1334, no new warnings elsewhere, and Doc/tools/check-warnings.py --fail-if-regression --fail-if-improved exits 0.