[asterisk-dev] Change in testsuite[master]: sip_attended_transfer now supports pre-12 Asterisk versions.

[Ashley Sanders (Code Review)]

Ashley Sanders has posted comments on this change.

Change subject: sip_attended_transfer now supports pre-12 Asterisk versions.
......................................................................


Patch Set 2:

(4 comments)

A vast improvement over the previous approach. I think this version is much easier for the human eye to follow the logic and glean intent, without having to use tools or execute the code directly.

The only issues that I have raised have been minor issues for clarity of PEP8 compliance.

You are very close, like inches and goal. Awesome work =)

https://gerrit.asterisk.org/#/c/29/2/tests/channels/SIP/sip_attended_transfer/attended_transfer.py
File tests/channels/SIP/sip_attended_transfer/attended_transfer.py:

I think you should run pylint on this - you have a couple of places where the continuation indentation is not correct and you are missing docstrings for the class methods.


Line 141:         self.chans = []
I think you should go ahead and spell this out since it is on the class level.


Line 142:         self.final_bridge = 0
Being that this variable denotes how many times we received the AMI VarSet event for the expected channel/value combination and for the variable, BRIDGEPEER, I think a clearer name for this would be something like, "self.bridged_peers".


Line 153:         if numchans == 1:
        :             self.controller.call_carol()
        :         elif numchans == 2:
        :             self.controller.transfer_call()
> Since Asterisk 11 Bridge events are 'weird' (aka: confusing and prone to br
I agree - while authoring the tests, we should do everything we can to help us later when debugging a broken asterisk. Future us will thank you greatly for better log messages :p


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I958c52cebb94f9cfc8dc8ed81311ae62efb2679d
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes