Detailed documentation on Proxy by adding docstrings to many of the methods.

[zimmer]

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.

[zimmer]

.patch that shows the added docstrings

[Thijs Triemstra]

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

[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.

[zimmer]

Fixed proxy doc strings

[Wilfredo Sánchez Vega]

Copyright change is still there.

[Thijs Triemstra]

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

[Thijs Triemstra]

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

[Thijs Triemstra]

Applied the patch with some fixes and forced a new [proxy-docs-5513 build]. Up for review.

[Thijs Triemstra]

Meant ​this build.

[Tom Prince]

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.

[Thijs Triemstra]

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

[Thijs Triemstra]

Fixed most of the review points, I'll leave the others to someone with more knowledge about this part of the code.

[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

[Tom Prince]

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).

[Jonathan Jacobs]

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.

2. ProxyClientFactory.__init_ has an extraneous newline in its docstring.

3. I noticed some docstrings, near other changes, were changed to start with a capital, but Request.__init__ didn't get this treatment.

4. 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.

5. The docstring for a parameter in handleResponsePart is missing a period.

[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

[Richard Wall]

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.