Opened 10 years ago
Last modified 9 years ago
#5513 enhancement new
Detailed documentation on Proxy by adding docstrings to many of the methods.
Reported by: | zimmer | Owned by: | Tom Prince |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | documentation |
Cc: | jknight, zimmer | Branch: |
branches/proxy-docs-5513
branch-diff, diff-cov, branch-cov, buildbot |
Author: | zimmer, thijs |
Description
The Proxy classes in proxy.py have either no docstring or generic simple ones that don't really help someone who is using it for the first time... I have added more detailed docstrings that hopefully help people who want to use twisted for proxies (as I did/do).
Basically the proxy.py documentation is sorely missing and hopefully this is a step on improving it.
Attachments (2)
Change History (22)
comment:1 Changed 10 years ago by
Cc: | jknight added |
---|
Changed 10 years ago by
Attachment: | my-twisted-patch.patch added |
---|
comment:2 Changed 10 years ago by
Cc: | zimmer added |
---|---|
Owner: | set to zimmer |
comment:3 Changed 10 years ago by
Owner: | zimmer deleted |
---|
comment:4 follow-up: 5 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to zimmer |
Thanks for your contribution, zimmer.
- I'm not sure why the outdated copyright header is added but that change should be reverted.
- You start using incorrect epytext at some point with
C(str)
vs correctC{str}
- Whenever you reference code, enclose it in
C{}
orL{ProxyRequest}
for references
comment:5 Changed 10 years ago by
Replying to thijs:
Thanks for your contribution, zimmer.
- I'm not sure why the outdated copyright header is added but that change should be reverted.
- You start using incorrect epytext at some point with
C(str)
vs correctC{str}
- Whenever you reference code, enclose it in
C{}
orL{ProxyRequest}
for references
My bad, I apologise I simple got the doc syntax by looking at client.py and must have read {} as (). I have changed those mistakes and made a new patch.
comment:6 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | zimmer deleted |
comment:7 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to zimmer |
Copyright change is still there.
comment:8 Changed 9 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/proxy-docs-5513 |
(In [37210]) Branching to 'proxy-docs-5513'
comment:9 Changed 9 years ago by
comment:10 Changed 9 years ago by
Author: | thijs → zimmer, thijs |
---|---|
Keywords: | review added |
Owner: | zimmer deleted |
Applied the patch with some fixes and forced a new [proxy-docs-5513 build]. Up for review.
comment:12 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
ProxyClient.__init__
: The docstring should primarily document what the method does and/or how to use it, rather than what uses it. (The later information can be included, if it useful, but I'm not sure it is in this case). I think the docstring (or the class docstring) should say something about how this forwarding received data to thefather
attribute.ProxyRequest.__init__
- The
reactor
description wording at the class level is much better than the__init__
version. channel
is just needs to be aHTTPChannel
(although in practice it will be aProxy
). Also, it isn't "used as", it "is" (that is, delete "used as".
- The
ReverseProxy
shouldn't point atexamples/proxy.py
. (Although pointing at the howto might be better than an example).- The added paragraph in
Proxy
doesn't seem clear to me. Maybe instead say something about being an http channel that usesProxyRequest
to handle requests, which then proxies things. ProxyRequest.process
- The parenthetical example is not clear worded. Either delete it, or move and expand it to a separate paragraph.
- The second paragraph could usefully point at
doc/web/examples/logging-proxy.py
. Also, the example given there is persuasive. Instead, logging or limiting allowed proxy destinations would be better, I think.
father
: Rather than saying where it comes from, the docstring should say that it is the request to forward the response to. (Also, it only needs to beRequest
, not necessarily aProxyRequest
.- "Object providing <interface>" can be shortened to "<interface>" in
@type
annotations.
Please resubmit for review, after addressing the above issues.
comment:14 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
Fixed most of the review points, I'll leave the others to someone with more knowledge about this part of the code.
comment:15 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
Removing from review, as the changes don't appear to deal with the more substantive review comments. I'll have a look at dealing with them
comment:16 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Tom Prince deleted |
Note ReverseProxy
is broken (see #5417).
I addressed the review points, and made some other improvements.
In particular, I removed some docstrings from ReverseProxyClient
where the documentation makes more sense to live on the super class (and improved those superclass docs that where missing).
comment:17 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
- I was under the impression that documentation parameters on init, that were stored with the same name on the instance, were to be documented as
ivar
in the class docstring. For examplefather
,command
, etc. onProxyClientFactory
andProxyClient
.
ProxyClientFactory.__init_
has an extraneous newline in its docstring.
- I noticed some docstrings, near other changes, were changed to start with a capital, but
Request.__init__
didn't get this treatment.
- Some documentation has been changed to refer to builtins (mostly
bytes
anddict
) withC{}
while some usesL{}
. SinceL{}
can now link to builtins, I think this style should be preferred.
- The docstring for a parameter in
handleResponsePart
is missing a period.
comment:18 Changed 9 years ago by
I was under the impression that documentation parameters on init, that were stored with the same name on the instance, were to be documented as ivar in the class docstring. For example father, command, etc. on ProxyClientFactory and ProxyClient.
We recently clarified this: https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9
comment:19 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Tom Prince deleted |
comment:20 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
Thanks tom, thjis, zimmer,
The added docstrings are a great improvement.
Here are one or two comments and suggestions:
- branches/proxy-docs-5513/twisted/web/http.py
- "489 @type buffer: L(bytes) ": Use curly instead of round brackets.
- And the parameter in "handleResponsePart" is actually called "data"
- "515 @param value": the parameter is called "val"
- "603 @param channel: The channel we're connected to. ": Sounds odd. Why not something like "The L{HTTPChannel} through which this L{Request} was received.
- "606 @param queued: Is this request...": I don't like using a question for attribute documentation. How about "A flag which - when set to C{True} - means that this Request is in the request queue. When set to C{False} means that this Request / Response can write to its channel.
- "813 Method called to process completed request. " >> "process *a/the* completed request"
- branches/proxy-docs-5513/twisted/web/proxy.py
- "44 @type command: L{bytes} ": Do you need to specify the type for ivar when it references "init"? The coding standard doesn't do this. Same for other ivars
- "67 @param rest: Rest of url": The existing variable name is
awful, but the new description could improve things by using
proper URL terminology. eg "The URL path, querystring, etc"
- "ex" should be "eg"
- "C{example.com/test.html}" isn't code so should be I{} I think
- "other than host" should be "excluding URL scheme and host" (if you choose to keep that sentence).
- "70 @type version: L{bytes}": Is it really bytes or an integer? In any case this parameter is never used (AFAICS) which seems like a bug. Raise a ticket if you agree.
- "75 (or through)": seems unnecessary and
confusing until the proxy code supports CONNECT ....but even
then I don't think any headers would be supplied.
- The "(or through)" clause is used repeatedly and I think it should be removed in all cases.
- "father": This strange variable name really confused and
distracted me (is father used because it's part of some parent
proxy feature). So I recommend that the new documentation
should be extra clear that this is the "client request" "from a
client on whose behalf to issue a request to an origin server"
- I'm not exactly sure how to say it but I think it needs to be clearer.
- ProxyClientFactory: Many of the comments above apply to these new docstrings too.
- "171 def buildProtocol(self, addr): ": Missing @param and @type addr.
- "324 L{ReverseProxy} is an implemention of HTTP": should probably be "implementation of an HTTP server"
- I think it would help to quote and link to the RFC2616 proxy definition (https://tools.ietf.org/html/rfc2616#section-1.3) in the module header and to various more specific sections in the proxy classes.
- Maybe this warrants a .doc newsfile?
Please resubmit for review after addressing or answering the numbered points above. Please also post a link to some clean build results.
Thanks again.
-RichardW.
.patch that shows the added docstrings