[Asterisk-code-review] StatsD: Write Testsuite tests (testsuite[master])

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

Change subject: StatsD: Write Testsuite tests
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/1392/3/tests/apps/statsd/mockd.py
File tests/apps/statsd/mockd.py:

Line 22: '''Protocol for the Mock Server to use for receiving messages.
       :     '''
If you are going to use a single-line docstring, it should all fit on one line, per the Pep 8 standards (https://www.python.org/dev/peps/pep-0257/#one-line-docstrings).


Also, per the guidelines (https://www.python.org/dev/peps/pep-0257/#what-is-a-docstring):

    For consistency, always use """triple double   
    quotes""" around docstrings. Use r"""raw triple double 
    quotes""" if you use any backslashes in your 
    docstrings. For Unicode docstrings, use u"""Unicode 
    triple-quoted strings""" .


Line 34: AMI Newexten event handler
Is this really handling the AMI Newexten event? It looks more likely to be an override for the base class DatagramProtocol->datagramReceived method:(http://twistedmatrix.com/documents/current/api/twisted.internet.protocol.AbstractDatagramProtocol.html#datagramReceived)



I know this is nitpicky, but the first statement should be a full-sentence, ending in a period, that prescribes the function or method's effect as a command (e.g. Handles event 'x'." (This is also per the standards (https://www.python.org/dev/peps/pep-0257/#one-line-docstrings)


Line 42: 'stasis.message'
This may need to be revisited once you get your dialplan application up and running such that you are able to discern the structure of the message that you receive. 

The reason I bring this up is that the other res_statsd messages are all predicated with 'stasis.message' which, seems logical to conclude, that the ones leaving your dialplan application would also be subject to the predicate, since their next stop after your application will be the res_statsd module.


Line 63: self.test_object.register_ami_observer(self.ami_connect)
       :         self.test_object.register_stop_observer(self._stop_handler)
Is there any reason you made one handler public and the other one private? 

(Caveat: by private, I really mean 'pseudo-private' as privacy isn't a 'right' for Python citizens, well, not without putting forth a bit of work to get there. FYI - the convention of using a single underscore before an attribute in Python (e.g. _<name>) only tells other developers to stay away from this even though they're not technically prevented from doing so.)


Line 97:     def message_received(self, message):
"What's in a name? That which we call a rose
By any other name would smell as sweet." -- Romeo and Juliet, Shakespeare

In the case of writing solid code, Shakespeare would be wrong. A name can be everything.

I really think the name of this method should be something like: message_handler. The reason being is that this is not the piece that 'receives' the message - the DatagramProtocol implementation is the responsible party for receiving messages. This method is simply processing them. 

I know this may at first seem like it's 'bikeshedding', but I think it's important to learn this lesson early, that names can be crucial for the next developer (who sometimes may even be yourself) to quickly glean intent such that they can debug a failure. You will thank the person who authored the pieces that you are mentally traversing, if they gave those methods/functions/properties a name that accurately depicts their purpose.


Line 112:     	Keyword Arguments:
        :     	ami -- The ami instance that detected the user event
        :     	event -- The user event that was detected
        : 
        :     	Pass along a user event to check_message for validation
        :     	'''
You have tabs here instead of spaces.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id93dbeea53cf978461151e9002af2afe1029ce8b
Gerrit-PatchSet: 3
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Tyler Cambron <tcambron at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Tyler Cambron <tcambron at digium.com>
Gerrit-HasComments: Yes