[asterisk-dev] [Code Review] 2846: Finish conversion script from sip.conf to pjsip.conf

[Mark Michelson]

> On Sept. 30, 2013, 4:19 p.m., Matt Jordan wrote:
> >

In most cases, you've commented on stuff that I did not actually add in this review. Some of these questions I'm not going to know the answer to, so I'll direct Kevin to make a response in those places so things can be made more clear.


> On Sept. 30, 2013, 4:19 p.m., Matt Jordan wrote:
> > /trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py, lines 40-41
> > <https://reviewboard.asterisk.org/r/2846/diff/2/?file=46653#file46653line40>
> >
> >     I'm not a fan of parameters that should be mutually exclusive coexisting.
> >     
> >     For example, it is invalid to call this function as follows:
> >     
> >     my_dict.get(my_key, from_self=False, from_templates=True, from_defaults=True)
> >     
> >     The fact that from_templates has priority over from_default can only be inferred from its order in the parameter list.
> >     
> >     While python doesn't really support enumerations, you can 'fake it' with class level attributes. Something like:
> >     
> >     class KeySource(object):
> >       OBTAIN_FROM_SELF = 0
> >       OBTAIN_FROM_TEMPLATES = 1
> >       OBTAIN_FROM_DEFAULTS = 2
> >     
> >     class Section(MultiOrderedDict):
> >     
> >       def get(self, key, key_source=KeySource.OBTAIN_FROM_SELF):
> >         ...
> >     
> >     And then throw an exception if the key source is not one of the allowed values.
> >

Agreed that the documentation could use a lot of work on this function.

Your suggestion for combining parameters would change the semantics of the function as it currently exists though. For instance, the example that you said is invalid is potentially valid. If the t.get() call in the from_templates block were to throw KeyErrors on every iteration, then the from_defaults block would be entered next.

Kevin, can you comment on whether from_self, from_default, and from_template are meant to be mutually exclusive or not?


> On Sept. 30, 2013, 4:19 p.m., Matt Jordan wrote:
> > /trunk/contrib/scripts/sip_to_pjsip/sip_to_pjsip.py, lines 111-118
> > <https://reviewboard.asterisk.org/r/2846/diff/2/?file=46655#file46655line111>
> >
> >     Remove is_in; use elif:
> >     
> >     if 'yes' in val:
> >     
> >     elif 'comedia' in val:
> >     
> >     ...

Can't use elif here since this value is a comma-separated list of NAT values. So both 'comedia' and 'force_rport' may be present. I'll switch to not use "is_in" but will not switch to elif.


> On Sept. 30, 2013, 4:19 p.m., Matt Jordan wrote:
> > /trunk/contrib/scripts/sip_to_pjsip/sip_to_pjsip.py, lines 138-144
> > <https://reviewboard.asterisk.org/r/2846/diff/2/?file=46655#file46655line138>
> >
> >     Same here

As with the previous comment, elif won't work here since multiple values may be present.


> On Sept. 30, 2013, 4:19 p.m., Matt Jordan wrote:
> > /trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py, lines 87-110
> > <https://reviewboard.asterisk.org/r/2846/diff/2/?file=46653#file46653line87>
> >
> >     I'm confused. Why do we reverse the sorted lists, then reverse them again when searching?

Kevin, can you comment on this one?


> On Sept. 30, 2013, 4:19 p.m., Matt Jordan wrote:
> > /trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py, lines 227-247
> > <https://reviewboard.asterisk.org/r/2846/diff/2/?file=46653#file46653line227>
> >
> >     It feels exceedingly odd that these are public functions with no relationship to a class when the first parameter is always a MultiOrderedConfigParser. These should probably be a public functions on that class.

My guess is that this was written this way originally because parser could be of different types. The exception handling is there likely for catching AttributeErrors in the try block. Since in actuality only one type is ever given, this can be rewritten to be a member of MultiOrderedConfigParser.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2846/#review9859
-----------------------------------------------------------


On Sept. 26, 2013, 9 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2846/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2013, 9 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22374
>     https://issues.asterisk.org/jira/browse/ASTERISK-22374
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This finishes off the initial work on the conversion script from sip.conf to pjsip.conf. This addresses missing endpoint options, registration options, auth options, and transport options.
> 
> I ran the flake8 checker on the files, but I only corrected its complaints in the new sections I added in order to keep the diff reduced to the relevant changes. Unfortunately, this ended up being moot since I renamed the files and the directory that they are in.
> 
> 
> Diffs
> -----
> 
>   /trunk/contrib/scripts/sip_to_pjsip/astdicts.py PRE-CREATION 
>   /trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py PRE-CREATION 
>   /trunk/contrib/scripts/sip_to_pjsip/sip_to_pjsip.py PRE-CREATION 
>   /trunk/contrib/scripts/sip_to_res_sip/astconfigparser.py 399909 
>   /trunk/contrib/scripts/sip_to_res_sip/astdicts.py 399909 
>   /trunk/contrib/scripts/sip_to_res_sip/sip_to_res_sip.py 399909 
> 
> Diff: https://reviewboard.asterisk.org/r/2846/diff/
> 
> 
> Testing
> -------
> 
> Tested against various sip.conf files to ensure that generated sections had the expected data in them.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131014/b8218b53/attachment-0001.html>