[Asterisk-code-review] Adds option to send crash reports via email (testsuite[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: Adds option to send crash reports via email
......................................................................


Patch Set 4: Code-Review-1

(1 comment)

https://gerrit.asterisk.org/#/c/1539/4/lib/python/mailer.py
File lib/python/mailer.py:

Line 28:     Note: This function uses print and is intended to be used outside the
       :     context of reactor.
       :     """
       :     if not SMTP_AVAILABLE:
       :         print "smtplib could not be loaded, email functionality is unavailable"
       :         return -1
       : 
       :     subject = message.get('subject')
       :     if subject:
       :         email_text = "Subject: {0}\n\n{1}".format(subject,
       :                                                   message['body'])
       :     else:
       :         email_text = message['body']
       : 
       :     try:
       :         smtp_obj = smtplib.SMTP(server)
       :         smtp_obj.set_debuglevel(debug)
       :         smtp_obj.sendmail(sender, recipients, email_text)
       :     except smtplib.SMTPServerDisconnected:
       :         print "Failed to connect to SMTP Server"
       :         return -1
       :     except smtplib.SMTPException as smtp_exception:
       :         print "SMTP Error: {0}".format(smtp_exception)
       :         return -1
       : 
       :     return 0
You should not mix code that works with exceptions along with return codes. Mixing the two together is never a good idea.

And the fact is, you are mixing the two here. What happens if this:

    try:
        smtp_obj = smtplib.SMTP(server)
        smtp_obj.set_debuglevel(debug)
        smtp_obj.sendmail(sender, recipients, email_text)
    except smtplib.SMTPServerDisconnected:
        print "Failed to connect to SMTP Server"
        return -1
    except smtplib.SMTPException as smtp_exception:
        print "SMTP Error: {0}".format(smtp_exception)
        return -1

Raises some exception other than smtplib.SMTPServerDisconnected or smtplib.SMTPException?

If I *never* want e-mailing backtraces Now my caller has to do the following:

try:
    if send_email < 0:
        print "I guess that didn't work"
except:
    print "I guess that didn't work"

Which is clearly not good.


Either your function handles all the exceptions, or the caller does. Since the libraries you're calling *will* raise exceptions, you should *not* be converting that into some error return code.

Again, as I said, this is not C. Don't try to make it C. Work in the exception driven world you're in.


-- 
To view, visit https://gerrit.asterisk.org/1539
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I17def41f15294a48754269d9c42aa649389840ad
Gerrit-PatchSet: 4
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes