Opened 6 years ago

Last modified 4 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)

my-twisted-patch.patch (7.3 KB) - added by zimmer 6 years ago.
.patch that shows the added docstrings
proxydocs2.patch (7.3 KB) - added by zimmer 6 years ago.
Fixed proxy doc strings

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: jknight added

Changed 6 years ago by zimmer

Attachment: my-twisted-patch.patch added

.patch that shows the added docstrings

comment:2 Changed 6 years ago by zimmer

Cc: zimmer added
Owner: set to zimmer

comment:3 Changed 6 years ago by Thijs Triemstra

Owner: zimmer deleted

comment:4 Changed 6 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to zimmer

Thanks for your contribution, zimmer.

  1. I'm not sure why the outdated copyright header is added but that change should be reverted.
  2. You start using incorrect epytext at some point with C(str) vs correct C{str}
  3. Whenever you reference code, enclose it in C{} or L{ProxyRequest} for references

comment:5 in reply to:  4 Changed 6 years ago by zimmer

Replying to thijs:

Thanks for your contribution, zimmer.

  1. I'm not sure why the outdated copyright header is added but that change should be reverted.
  2. You start using incorrect epytext at some point with C(str) vs correct C{str}
  3. Whenever you reference code, enclose it in C{} or L{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.

Changed 6 years ago by zimmer

Attachment: proxydocs2.patch added

Fixed proxy doc strings

comment:6 Changed 6 years ago by Thijs Triemstra

Keywords: review added
Owner: zimmer deleted

comment:7 Changed 6 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to zimmer

Copyright change is still there.

comment:8 Changed 5 years ago by Thijs Triemstra

Author: thijs
Branch: branches/proxy-docs-5513

(In [37210]) Branching to 'proxy-docs-5513'

comment:9 Changed 5 years ago by Thijs Triemstra

(In [37211]) apply my-twisted-patch.patch, add news file. refs #5513

comment:10 Changed 5 years ago by Thijs Triemstra

Author: thijszimmer, 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:11 Changed 5 years ago by Thijs Triemstra

Meant this build.

comment:12 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. 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 the father attribute.
  2. ProxyRequest.__init__
    • The reactor description wording at the class level is much better than the __init__ version.
    • channel is just needs to be a HTTPChannel (although in practice it will be a Proxy). Also, it isn't "used as", it "is" (that is, delete "used as".
  3. ReverseProxy shouldn't point at examples/proxy.py. (Although pointing at the howto might be better than an example).
  4. The added paragraph in Proxy doesn't seem clear to me. Maybe instead say something about being an http channel that uses ProxyRequest to handle requests, which then proxies things.
  5. 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.
  6. 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 be Request, not necessarily a ProxyRequest.
  7. "Object providing <interface>" can be shortened to "<interface>" in @type annotations.

Please resubmit for review, after addressing the above issues.

comment:13 Changed 5 years ago by Thijs Triemstra

(In [37280]) address review comments, refs #5513

comment:14 Changed 5 years ago by Thijs Triemstra

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 5 years ago by Tom Prince

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 5 years ago by Tom Prince

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 5 years ago by Jonathan Jacobs

Keywords: review removed
Owner: set to Tom Prince
  1. 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.
  1. ProxyClientFactory.__init_ has an extraneous newline in its docstring.
  1. I noticed some docstrings, near other changes, were changed to start with a capital, but Request.__init__ didn't get this treatment.
  1. Some documentation has been changed to refer to builtins (mostly bytes and dict) with C{} while some uses L{}. Since L{} can now link to builtins, I think this style should be preferred.
  1. The docstring for a parameter in handleResponsePart is missing a period.

comment:18 Changed 5 years ago by Jean-Paul Calderone

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 4 years ago by Tom Prince

Keywords: review added
Owner: Tom Prince deleted

comment:20 Changed 4 years ago by Richard Wall

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:

  1. branches/proxy-docs-5513/twisted/web/http.py
    1. "489 @type buffer: L(bytes) ": Use curly instead of round brackets.
    2. And the parameter in "handleResponsePart" is actually called "data"
    3. "515 @param value": the parameter is called "val"
    4. "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.
    5. "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.
    6. "813 Method called to process completed request. " >> "process *a/the* completed request"
  2. branches/proxy-docs-5513/twisted/web/proxy.py
    1. "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
    2. "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"
      1. "ex" should be "eg"
      2. "C{example.com/test.html}" isn't code so should be I{} I think
      3. "other than host" should be "excluding URL scheme and host" (if you choose to keep that sentence).
    3. "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.
    4. "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.
      1. The "(or through)" clause is used repeatedly and I think it should be removed in all cases.
    5. "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"
      1. I'm not exactly sure how to say it but I think it needs to be clearer.
    6. ProxyClientFactory: Many of the comments above apply to these new docstrings too.
    7. "171 def buildProtocol(self, addr): ": Missing @param and @type addr.
    8. "324 L{ReverseProxy} is an implemention of HTTP": should probably be "implementation of an HTTP server"
  3. 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.
  4. 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.

Note: See TracTickets for help on using tickets.