Opened 7 years ago

Closed 6 years ago

#6739 enhancement closed fixed (fixed)

relay.py and relaymanager.py should conform to docstring and whitespace standards

Reported by: Stacey Sern Owned by: Stacey Sern
Priority: normal Milestone:
Component: mail Keywords: documentation
Cc: Branch: branches/relay-doc-6739
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

Description (last modified by hawkowl)

The docstrings in relay.py and relaymanager.py should be updated to provide descriptions of all classes, functions and methods as well as their parameters and attributes. The spacing before classes and functions should conform to the coding standards.

Some other tickets involve changing code in these files so some docstrings will be changed under those tickets instead of this one. The changes omitted from this ticket are:

relay.py

  • #6383: DomainQueuer, DomainQueuer.init and DomainQueuer.willRelay
  • #6717: All docstrings for RelayerMixin

relaymanager.py

  • #6717: All docstrings for ManagedRelayerMixin
  • #6719: SmartHostSMTPRelayingManager._checkStateMX
  • #6705: All docstrings for _AttemptManager

Change History (23)

comment:1 Changed 7 years ago by Stacey Sern

Author: shira
Branch: branches/relay-doc-6739

(In [39958]) Branching to 'relay-doc-6739'

comment:2 Changed 7 years ago by Stacey Sern

The list above should read:

relay.py

  • #6383: DomainQueuer, DomainQueuer.init and DomainQueuer.willRelay
  • #6719: All docstrings for RelayerMixin

relaymanager.py

  • #6719: All docstrings for ManagedRelayerMixin
  • #6717: SmartHostSMTPRelayingManager._checkStateMX
  • #6705: All docstrings for _AttemptManager

comment:3 Changed 7 years ago by Stacey Sern

(In [40017]) Revise docstrings and whitespace

refs #6739

comment:4 Changed 7 years ago by Stacey Sern

(In [40018]) Add topfile

refs #6739

comment:5 Changed 7 years ago by Stacey Sern

(In [40019]) Revise topfile

refs #6739

comment:6 in reply to:  2 Changed 7 years ago by hawkowl

Description: modified (diff)

comment:7 in reply to:  2 Changed 7 years ago by hawkowl

Description: modified (diff)

comment:8 Changed 7 years ago by Stacey Sern

Keywords: review added
Owner: changed from Stacey Sern to hawkowl

comment:9 Changed 7 years ago by hawkowl

Keywords: review removed
Owner: changed from hawkowl to Stacey Sern

Hi shira, thanks for working on this! Sorry for the few days delay!

  1. Queue.getPath() (line 432) has no @return or @rtype.
  1. Subjectively: I don't like the second part of relaymanager.py's docstring. It does seem to work as is, so it's fine to leave it if you disagree, but it did take reading it a few times for me to actually get what it was talking about.
  1. Subjectively: In SmartHostSMTPRelayingManager.__init__, I don't particularly like the wording of "Default values are meant for a small box with 1-5 users.", since this sounds like it has performance implications, rather than the small connections at once. Maybe add that it's the connection limits that would support 1-5 users best?

Please address #1 and resubmit, with anything for 2 and 3 if you agree. Thanks again.

comment:10 Changed 6 years ago by Stacey Sern

(In [40251]) Incorporate code review comments

refs #6739

comment:11 Changed 6 years ago by Stacey Sern

(In [40253]) Fix trailing whitespace

refs #6739

comment:12 Changed 6 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

Fixed item 1. Revised comments on 2 and 3. Please let me know if you think they are better.

comment:13 Changed 6 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to Stacey Sern

There are also quite a few occurences of wrong spacing:

return {'directory' : self.directory}

vs

return {'directory': self.directory}

and:

message+'-H'

vs

message + '-H'

I understand this isn't a docstring issue but its about whitespace. Can you fix that whitespace as well to make the code more readable?

comment:14 Changed 6 years ago by Stacey Sern

(In [40518]) Fix additional whitespace issues and long lines

refs #6739

comment:15 Changed 6 years ago by Stacey Sern

(In [40519]) Revert changes to web/_newclient.py

refs #6739

comment:16 Changed 6 years ago by Stacey Sern

(In [40520]) Remove trailing whitespace

refs #6739

comment:17 Changed 6 years ago by Stacey Sern

Keywords: review documentation added
Owner: Stacey Sern deleted

comment:18 Changed 6 years ago by Hynek Schlawack

Keywords: review removed
Owner: set to Stacey Sern
  1. I’ve forced a build, everything looks happy, no pydoctor errors within relay(manager)?.py.
  2. relay.py
    1. 39: maybe “a … provider” would be better?
  3. relaymanager.py
    1. @type secret: See L{__init__} doesn’t work, I think you should use @ivar secret: see L{__init__} (occurs several times).
    2. 448: I suppose it’s a list of 2-tuples? I think that should be explicitly noted as in the other cases.
    3. 640: I find this 1–5 users somewhat problematic since it depends on the machine and the amount of mail those 1–5 users receive. Maybe just call it “a personal low-volume smart host”? This one is optional and should probably be discussed with someone with more insight.
    4. 939: would you consider “which successfully fires with L{Record_MX}” really part of the type? Do we this elsewhere too? I’d just keep it in the @return.
    5. 963: provider*s* seems more apt here I think
    6. 982: same
    7. 1068: there is both “MX record” and “mail exchange record” in one sentence. I’d prefer if it were consistent.

Since it has been already proof-read by a native speaker and those points are rather minor, you can merge after fixing 3.1 + 3.2 + whatever you consider fix-worthy as a native speaker yourself.

comment:19 Changed 6 years ago by Stacey Sern

(In [41214]) Incorporate code review comments and update tuple and dict notation.

refs #6739

comment:20 Changed 6 years ago by Stacey Sern

Keywords: review added

3.1 Great catch. 3.2 It's a list of two bytes so I changed it to "L{list} of two L{bytes}" so it doesn't look like tuple notation. 3.3 I changed it to "low-volume smart host". 3.4 I think that when the return type of a deferred is meant to be used (as opposed to using the deferred to just wait for something to happen), it should be part of the type. The return value of getMX is really RecordMX. It's just not immediately available so the deferred is returned. If that part was only included in the text of the @return, important detailed information would be missing. 3.5&3.6 I agree that in terms of English, plural seems better here. For better or worse, the style that I used throughout uses the type in singular as in L{list} of L{int}. Among other reasons, the s would have to go outside of the braces which looks a little funny and may render in different size type in the API documentation. 3.7 Done.

I also changed the tuple and dict notation in response to comments on other mail documentation tickets. Because of this change I am putting the ticket back into review but if they are considered minor enough I can merge.

comment:21 Changed 6 years ago by Hynek Schlawack

Keywords: review removed

I don’t see the low-volume fix, otherwise this looks fine, please merge (after fixing the 1–5/low-volume thing if you are inclined to do so).

comment:22 Changed 6 years ago by Stacey Sern

(In [41215]) Change docstring wording.

refs #6739

comment:23 Changed 6 years ago by Stacey Sern

Resolution: fixed
Status: newclosed

(In [41216]) Merge <relay-doc-6739: Fix docstrings for relay.py and relaymanager.py

Author: shira Reviewer: hawkowl, thijs, hynek Fixes: #6739

Note: See TracTickets for help on using tickets.