Opened 3 years ago

Last modified 15 months ago

#5685 enhancement new

SMTP Client Tutorial should be self-contained

Reported by: argonemyth Owned by: shira
Priority: lowest Milestone:
Component: mail Keywords: documentation
Cc: jesstess, fei@…, thijs, staceysern@… Branch: branches/smtp-tutorial-5685
(diff, github, buildbot, log)
Author: tomprince Launchpad Bug:

Description

The tutorial is nice. I like the error-prone to the exception-free approach, but the current twistd no longer throws those exceptions. Running smtpclient-1.tac to smtpclient-10.tac, you would get similar output! So, a re-write is needed! :)

Attachments (4)

5685-SMTPClient_Tutorial-20120724.patch (17.9 KB) - added by argonemyth 2 years ago.
5685_2.patch (24.2 KB) - added by sdsern 18 months ago.
5685_2.2.patch (24.2 KB) - added by sdsern 18 months ago.
5685_3.patch (30.0 KB) - added by sdsern 18 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 2 years ago by argonemyth

  • Cc fei@… added
  • Keywords review added
  • Owner argonemyth deleted
  • Priority changed from normal to lowest
  • Type changed from defect to enhancement

Hi,

I was totally wrong about the twistd's behaviour described above!!! I must have ran the tac files without checking what's inside, and I didn't have a SMTP server on the localhost. So, the only change I made to the tutorial itself is to add the following sentences in the SMTP Client 3 section:

If you don't have a local SMTP server, you can replace localhost with other SMTP servers (like ASPMX.L.GOOGLE.COM). The rest of the tutorial will require a working SMTP server.

I also updated the output with Twisted version 12.1.0 and removed some trailing whitespace.

It's ok if you don't use the patch, but still, running all the examples and updating the output did took quite some time, so I would be glad if you guys use the patch!!

Thanks!

Changed 2 years ago by argonemyth

comment:2 Changed 2 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to argonemyth
  1. The output almost matches the actual example output. In the tutorial I see:
    18:55 EST [-] Log opened.
    
    Running it locally shows the full timestamp is missing:
    2012-09-13 14:51:18+0200 [-] Log opened.
    
  2. When running twistd -ny smtpclient-3.tac it outputs the following, which differs from the output you added, why is that?:
    2012-09-13 14:54:42+0200 [-] Log opened.
    2012-09-13 14:54:42+0200 [-] using set_wakeup_fd
    2012-09-13 14:54:42+0200 [-] twistd 12.1.0+r35620 (/usr/bin/python 2.7.3) starting up.
    2012-09-13 14:54:42+0200 [-] reactor class: twisted.internet.epollreactor.EPollReactor.
    2012-09-13 14:54:42+0200 [-] Starting factory <twisted.internet.protocol.ClientFactory instance at 0x7fe356107ea8>
    2012-09-13 14:54:42+0200 [Uninitialized] Stopping factory <twisted.internet.protocol.ClientFactory instance at 0x7fe356107ea8>
    ^C2012-09-13 14:54:48+0200 [-] Received SIGINT, shutting down.
    2012-09-13 14:54:48+0200 [-] Main loop terminated.
    2012-09-13 14:54:48+0200 [-] Server Shut Down.
    
  3. Same for smtpclient-5.tac, I don't see the traceback.

In general:

  1. A .misc newsfile is missing
  2. Can you put this in a branch so we can test it with buildbot

Thanks!

comment:3 Changed 18 months ago by sdsern

  • Owner changed from argonemyth to sdsern

comment:4 Changed 18 months ago by sdsern

  • Summary changed from Outdated SMTP Client Tutorial to SMTP Client Tutorial should be self-contained

In order for the tutorial to be run successfully now, there must be an SMTP server running on localhost:25 but the tutorial does not explicitly say that.

The tutorial should give directions for setting up an SMTP server and anything else that is required for the client programs to work. The user should be able to run through the tutorial successfully without needed any external knowledge of SMTP.

Changed 18 months ago by sdsern

Changed 18 months ago by sdsern

comment:5 Changed 18 months ago by sdsern

  • Cc staceysern@… added
  • Keywords review added

5685_2.patch updates the SMTP Client Tutorial so that directions for everything that is necessary to run the tutorial are included. Specifically, the tutorial instructs on how to run an SMTP server for the client to interact with. The examples now use port 8025 instead of port 25. Client #11 has been changed to use a hosts file instead of contacting a DNS server for the address of example.net. A new file, smtpclient-11a.tac, has been added as an alternative which contacts a DNS server for the address.

Other changes include:
Changed the header tags so that the examples are not under the Introduction.
Added a summary section.
Added API links.
Changed class of command line code from python to shell.
Added note that exact output may vary from tutorial output.
Minor punctuation, grammar, clarity and accuracy changes.

comment:6 Changed 18 months ago by sdsern

  • Owner sdsern deleted

comment:7 Changed 18 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/smtp-tutorial-5685

(In [38948]) Branching to smtp-tutorial-5685.

comment:8 Changed 18 months ago by tomprince

(In [38949]) Apply 5685_2.2.patch from sdsern.

Refs: #5685

comment:9 Changed 18 months ago by tom.prince

  • Keywords changed from documentation, review to documentation review

Thanks for your work on this. This isn't a complete review, but just some initial thoughts. I'll try to do a fuller review tomorrow.

Some general comments on the overall structure of this tutorial:

  1. While I think the approach taken is interesting for teaching people about twisted, it doesn't really address how to write an smtp client. After reading this, I have some idea of how to write a tac file, but only a vauge understanding of how to send mail.
  2. I'm not sure that using twistd is really appropriate for writting a client. Using twisted.internet.task.react would be more appropriate.
    • endpoints would also be more appropriate in that case

Some comments on the current content:

  1. Consider following the policy outlined in #6537.
    • Note, this doesn't mean changing the entire tutorial, just lines that you are modifying anyway.
  2. I was going to suggest that extracts should use the py-listings facitily provided by lore, but I guess that that only supports skip initial lines, not arbitrary ranges. :(
    • And code blocks don't support specifying a starting line number either. :(
    • These might be good things to open tickets about.
  3. The shell code/example output are inconsitent in the presence absence of a prompt. Also, the path in the output prompt are improbable. At least for output examples, consider using $ as the prompt.

comment:10 Changed 18 months ago by exarkun

I was going to suggest that extracts should use the py-listings facitily provided by lore, but I guess that that only supports skip initial lines, not arbitrary ranges. :(
And code blocks don't support specifying a starting line number either. :(
These might be good things to open tickets about.

Line-number based inclusion is fragile. It is a a bad feature that makes maintaining documentation harder. Some other mechanism for limited inclusion might be interesting to explore, perhaps name-based.

comment:11 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to sdsern

And here is the rest of my review:

  1. There are a number of references to specific classes that could be api links (see the documentation standard for how to do this.
  2. Not only do some ISPs block outbound smtp, example.net doesn't accept mail. Thus, your smtpclient-11a.tac doesn't work, even on a network where port 25 isn't filtered. I think there are few options here:
    1. Just update the note to that effect.
    2. Show the code change necesarry to use real DNS without having a corresponding tac.
    3. Remove 11a entirely.

I'm not sure what is the best option, but probably 1 or 2.

  1. The bit about twistd should perhaps refer to running bin/twistd from a twisted checkout (at least as an option).
  2. Looking at twisted's writing standard:
    • The introduction heading is superfluous. (see here)

Please resubmit for review after addressing 3 and 5-9.

As exarkun suggested, 1+2 are scope creep, and can be addressed in subsequent tickets.

comment:12 Changed 18 months ago by sdsern

  • Keywords review added
  • Owner sdsern deleted

5685_3.patch introduces the following changes:

  1. Completely removes the command prompt lines from the output and the output in response to control-c. The output listings are now a bit cleaner looking since they are strictly the response to running the specified programs.
  1. Added a few more api links. When a class appears more than once, often only the first occurrence has a link.
  1. Added explanation to the tutorial and to smtpclient-11a.tac that the recipient's email address and the domain to lookup must be modified.
  1. Added a mention of bin/twistd
  1. Removed introduction heading
  1. Some other wording changes for American English spelling, clarity and grammar and removal of extraneous imports.
  1. Every paragraph that required a change was reformatted so that each sentence is its own line.

Changed 18 months ago by sdsern

comment:13 Changed 18 months ago by tomprince

(In [39019]) Apply 5685_3.patch from sdsern.

Refs: #5685

comment:14 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to sdsern

Thanks for your work on this.

  1. I think the following (while correct) may be apt to be misinterpreted.

To address this issue, Twisted provides Deferreds a mechanism for delaying action until an asynchronous request is complete.


When I first read it, it seemed to be saying that deferreds cause things to be delayed. (I've heard a number of people talk about "deferreds in the reactor"). I think it might be clearer to say something along the lines "specifying an action to take place when an asynchronous request is complete".

  1. The following seems to be missing some punctuation or linking words

    you'll need to make two changes to the file to supply an actual email address for the recipient and to look up the recipient's domain.

Please reword the above two sections, and resubmit for review.

comment:15 Changed 18 months ago by shira

  • Owner changed from sdsern to shira
  • Status changed from new to assigned

comment:16 Changed 17 months ago by shira

(In [39189]) Changed wording.

refs #5685

comment:17 Changed 17 months ago by shira

  • Keywords review added
  • Owner shira deleted
  • Status changed from assigned to new

comment:18 Changed 17 months ago by habnabit

For what it's worth, sphinx has a name-based source inclusion mechanism. See the 'pyobject' section of http://sphinx-doc.org/markup/code.html#includes

comment:19 Changed 15 months ago by rwall

  • Keywords review removed
  • Owner set to shira

Thanks shira,

This looks like a great improvement.

I'd say this tutorial belongs in the core howto section. I've never
seen it before and I imagine it could have been more useful than the
finger tutorial when learning Twisted. Its more about twisted
fundamentals than SMTP mail....but that's a different issue.

Notes:

Points:

  1. "By the time the tutorial is complete, you will understand" -- I'd prefer this to be followed by a bullet list.
  2. I get a different exception in step2
    2013-09-13 10:00:59+0100 [-]     if '%' in addr:
    2013-09-13 10:00:59+0100 [-] TypeError: argument of type 'NoneType' is not iterable
    
  3. During the review I had to hunt for the tac download links amongst the text -- It might be nice to add additional tac download links next to each heading.
  4. StringIO -- maybe use the io.BytesIO module for python3 compatibility.
  5. Conclusion -- maybe use a bullet list here too.
  6. Step 11 didn't actually work for me - the hosts path is wrong. Add the default unix path and a note about the host path on Windows. (https://twistedmatrix.com/documents/current/api/twisted.names.client.html#createResolver)
[richard@zorin doc]$ cat /etc/hosts
127.0.0.1		localhost.localdomain localhost example.net
::1		localhost6.localdomain6 localhost6
# 66.35.39.66     twistedmatrix.com speed.twistedmatrix.com
[richard@zorin doc]$ twistd -ny smtpclient.tac
2013-09-13 10:23:37+0100 [-] Log opened.
2013-09-13 10:23:37+0100 [-] twistd 13.0.0 (/usr/bin/python 2.7.5) starting up.
2013-09-13 10:23:37+0100 [-] reactor class: twisted.internet.epollreactor.EPollReactor.
2013-09-13 10:23:37+0100 [DNSDatagramProtocol (UDP)] MX lookup failed; attempting to use hostname (example.net) directly
2013-09-13 10:23:37+0100 [DNSDatagramProtocol (UDP)] DNSDatagramProtocol starting on 52440
2013-09-13 10:23:37+0100 [DNSDatagramProtocol (UDP)] Starting protocol <twisted.names.dns.DNSDatagramProtocol object at 0x18b3c10>
2013-09-13 10:23:37+0100 [-] (UDP Port 53129 Closed)
2013-09-13 10:23:37+0100 [-] Stopping protocol <twisted.names.dns.DNSDatagramProtocol object at 0x18b3450>
2013-09-13 10:23:37+0100 [-] (UDP Port 52440 Closed)
2013-09-13 10:23:37+0100 [-] Stopping protocol <twisted.names.dns.DNSDatagramProtocol object at 0x18b3c10>
^C2013-09-13 10:23:41+0100 [-] Received SIGINT, shutting down.
2013-09-13 10:23:41+0100 [-] Main loop terminated.
2013-09-13 10:23:41+0100 [-] Server Shut Down.

I had to make the following change.

[richard@zorin doc]$ bzr diff
=== modified file 'smtpclient.tac'
--- smtpclient.tac	2013-09-13 09:20:55 +0000
+++ smtpclient.tac	2013-09-13 09:26:28 +0000
@@ -47,7 +47,7 @@
 def getMailExchange(host):
     def cbMX(mxRecord):
         return str(mxRecord.name)
-    return (relaymanager.MXCalculator(createResolver(None, None, b"hosts"))
+    return (relaymanager.MXCalculator(createResolver(None, None, b"/etc/hosts"))
             .getMX(host).addCallback(cbMX))

 def cbMailExchange(exchange):

Alternatively (on unix) createResolver can be called without arguments
and it will default to standard unix hosts location.

Please fix point 6.

Address points 1-5 if you feel like it.

Then merge.

Thanks again.

-RichardW.

Note: See TracTickets for help on using tickets.