> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, lines 86-88 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line86> > > > > I think this is a good case for using "is" instead of equality. I > > imagine that callers of this function are going to be passing a reference > > to the same account object that is stored on the phone_obj rather than a > > separate object with equivalent properties. > > > > Also, to avoid the continue statement, you can just use a positive > > comparison instead of a negative one: > > > > if account is phone_obj.account: > > return phone_obj
Yup, that's what I meant to do :) > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, lines 115-117 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line115> > > > > A couple of PEP 8 points: > > > > 1) When catching exceptions, use the syntax "except ErrorType as > > ErrorInstance" instead of "except ErrorType, ErrorInstance". So here, it > > would be "except pj.Error as err" > > > > 2) A PEP 8 checker will complain about the indentation of str(err)) on > > the final line I've highlighted. It will claim it should be lined up like > > this: > > > > raise Exception("Exception occurred while making call: '%s'" % > > str(err)) > > > > See how "str" is lined up to show how it belongs to the parentheses? I blame my python plugin for vim which just shifts twice. I installed the pep8 utility which pointed out a couple others which I have also fixed. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, line 156 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line156> > > > > self.current_call doesn't seem to be used for anything. Removed. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, lines 190-191 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line190> > > > > Should this only be done if self.phone is None? Seems like this would > > be redundant after the initial setting. That would make sense. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, line 192 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line192> > > > > I suspect it's not an issue, but is there ever a chance that self.call > > could be None when the on_state() callback is called? As far as I know self.call would never be None when a pjsua callback is called such as on_state(). > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, lines 197-204 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line197> > > > > This seems overly complicated for what you are trying to do here. Why > > not just > > > > try: > > self.phone.calls.remove(self.call) > > except ValueError: > > # Log an error or something > > > > If that doesn't work because equality of calls doesn't evaluate as > > you'd expect, then this is a situation where I expect that a single object > > reference is being passed around everywhere rather than separate call > > objects. You could take the approach of reconstructing the self.phone.calls > > list like this: > > > > self.phone.calls = [call for call in self.phone.calls if call is not > > self.call] The first suggestion is actually what I initially was using. However I found that the call object wasn't always being removed from the list. Debugging showed the id() of self.call not matching any in the list which I believe is because self.call is set to weakref.proxy(call) in pjsua.CallCallback(). Comparing the SIP Call-ID of the call objects should be unique enough to find the one to remove from the list. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, line 246 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line246> > > > > This function's name is misleading because it does not actually log > > anything. It should either > > > > 1) Be renamed to something indicating that it is just gathering call > > information into a string > > > > or > > > > 2) Be altered to actually log the call info using LOGGER.debug() Renamed. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, line 270 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line270> > > > > Can you add a comment about why the lock is being used here? Fixed this by removing the locking :) Apparently a lock isn't needed when making call. Unless perhaps a callback isn't passed and then set later using call.set_callback() but that's not the case here. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/phones.py, lines 272-275 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line272> > > > > If this exception occurs, does lock get properly unlocked? Fixed this by removing the locking :) Apparently a lock isn't needed when making call. Unless perhaps a callback isn't passed and then set later using call.set_callback() but that's not the case here. > On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote: > > /asterisk/trunk/lib/python/asterisk/pjsua_mod.py, lines 258-259 > > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71349#file71349line258> > > > > Is this change necessary since you ended up overriding reg_success() in > > your new pluggable object? If that condition is removed then it will blow up if callback_module & callback_method are not defined in YAML and pjsua_mod.PJsua is being used as a test module. If it remains with the same scenario then things won't blow up and no user code is called into(maybe there's a need for that?). Suggestions? - jbigelow ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4413/#review14450 ----------------------------------------------------------- On Feb. 12, 2015, 10:25 a.m., jbigelow wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4413/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2015, 10:25 a.m.) > > > Review request for Asterisk Developers and Mark Michelson. > > > Bugs: ASTERISK-24578 > https://issues.asterisk.org/jira/browse/ASTERISK-24578 > > > Repository: testsuite > > > Description > ------- > > Pluggable modules to place, receive, and transfer (blind/attended) calls to > simulate phones using PJSUA and YAML configuration. Calls are placed and/or > transferred using the new pluggable action module. This should allow many > currrent and future tests to easily send/receive calls to/from Asterisk along > with transferring calls within YAML configuration. > > The pluggable test module (phones.PjsuaPhoneController) initializes the PJSUA > accounts and once all have registered, the account callbacks are setup and > are ready to receive calls. The pluggable action module > (pluggable_modules.PjsuaPhoneActionModule) provides the ability to place > calls and transfer calls using the accounts from YAML and the action is > referenced with 'pjsua_phone'. The only time a call is hung up by this is > when a transfer is performed and a 200 OK sipfrag NOTIFY is received. None of > the modules set a pass/fail result and are only for driving and manipulating > calls. > > See attached file for YAML demo. > > > Diffs > ----- > > /asterisk/trunk/lib/python/asterisk/pluggable_modules.py 6379 > /asterisk/trunk/lib/python/asterisk/pjsua_mod.py 6379 > /asterisk/trunk/lib/python/asterisk/phones.py PRE-CREATION > > Diff: https://reviewboard.asterisk.org/r/4413/diff/ > > > Testing > ------- > > * Tested placing calls, receiving calls, transfering via blind & attended. > * Pylint score of 9.40/10 for phones.py > * See attached test-config.yaml for a demonstration. > > > File Attachments > ---------------- > > Demonstration > > https://reviewboard.asterisk.org/media/uploaded/files/2015/02/11/659ab31f-8401-4f24-be5e-da1db0be3156__test-config.yaml > > > Thanks, > > jbigelow > >
-- _____________________________________________________________________ -- 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
