Mark Michelson has posted comments on this change.

Change subject: stasis: set a channel variable on websocket disconnect error
......................................................................


Patch Set 1: Code-Review+1

(4 comments)

I noticed that I gave the Code-Review a 0 last time. I figure that a +1 is 
actually in order since I noticed that my comments last time weren't about 
functional deficiencies so much as style and nitpicks. So here's my +1.

https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/ari_client.py
File tests/rest_api/applications/stasisstatus/ari_client.py:

Line 148:     def on_channelcreated(self, message):
> This function is what builds up the local container for channels created by
Ah, I missed the getattr() call in on_ws_event(). I'm going to blame gerrit's 
hijacking of my browser's search :-P


https://gerrit.asterisk.org/#/c/18/1/tests/rest_api/applications/stasisstatus/observable_object.py
File tests/rest_api/applications/stasisstatus/observable_object.py:

Line 84:             del self.__registrar[event][:]
> From the python documentation https://docs.python.org/3/tutorial/datastruct
Ah, thanks for the enlightenment!


Line 132:         if self.__suspended < 0:
        :             self.__suspended = 0
> Not including the = sign excludes this redundant case:
Correct, but my point was that if self.__suspended is 0 when entering this 
function, the result will be that it ends up being set to -1. My assumption was 
that you wanted self.__suspended to have a lower bound of 0. My suggestion was 
made in the spirit of brevity, but you could just as easily have

if self.__suspended == 0:
    return
elif self.__suspended > 0:
    self.__suspended -= 1
else:
    # WTF, how did it get to be < 0?


Line 143:     def __validate(self, **kwargs):
> It is so that there are more meaningful messages in the logfile when debugg
I wasn't suggesting getting rid of the method, just rewriting its body.

While DRY is fantastic, I'm also a firm believer in YAGNI, especially for tests 
that are concentrated on a specific case, where the data that is to be used is 
self-generated and known in advance. In this case, you've written a generic 
validation function that can take any number of named parameters, and based on 
the name of the parameter, perform some specific action. In actual use, the 
only parameter ever passed to this function is a single parameter called 
"event" (never multiple parameters), and it will never be NoneType (since the 
data being passed to this function is generated by your own test cases), and I 
don't actually think that any of the operations you perform could throw an 
IndexError anway. There's no reason to build a trans-oceanic cruise liner to 
cross a creek.

The error message argument makes sense. If you want to keep an error message in 
here, that's cool.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7dadfd429bd30e9f07a531f47884d8c923fc13
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asand...@digium.com>
Gerrit-Reviewer: Ashley Sanders <asand...@digium.com>
Gerrit-Reviewer: Mark Michelson <mmichel...@digium.com>
Gerrit-HasComments: Yes

-- 
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Reply via email to