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

[Matt Jordan]

> 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.
> >
> 
> Mark Michelson wrote:
>     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?
> 
> Kevin Harwell wrote:
>     They are not meant to be mutually exclusive.  If the key is not found in a section then it is suppose to search the next place it might be found unless that from_* flag is turned off.  However, priority is given to self, templates, and then defaults which should be the usual case.  I guess if one wants to change the priority they could call the function multiple times with the appropriate flags [un]set, or we could split into multiple functions.

Got it. Drop this finding.


- Matt


-----------------------------------------------------------
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/20131015/3945e116/attachment.html>