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

[Kevin Harwell]

> On Sept. 30, 2013, 11:19 a.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?

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.


> On Sept. 30, 2013, 11:19 a.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?
> 
> Mark Michelson wrote:
>     Kevin, can you comment on this one?

So yeah maybe a better way to do this, but before adding of templates the given list is sorted, reversed and then added to, so that the "oldest" template definitions are guaranteed to be last.  So, when a get/search is issued it looks in the most recently added templates first.  During merges it is reversed so the "oldest" is merged first.

Example:
[template_a](!)
allow=ulaw
allow=g711
context=default

Defined later at some point:

[template_a](!)
allow=alaw
context=test

a merge of those two should output: allow=ulaw,g711,alaw and not allow=alaw,ulaw,g711 while a search should "get" a context of "test".

Also see next issue's comment about why add_template should probably mimic add_defaults.


> On Sept. 30, 2013, 11:19 a.m., Matt Jordan wrote:
> > /trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py, lines 87-90
> > <https://reviewboard.asterisk.org/r/2846/diff/2/?file=46653#file46653line87>
> >
> >     This will add the elements in defaults to _defaults in reverse sorted order. So why not something like:
> >     
> >     self._defaults.extend(defaults.sort(reverse=True)
> >     
> >     
> >     Or exactly as add_templates was implemented.
> >

Actually I think it would be better to make add_templates do what add_defaults does instead since they will result in two different lists, but I believe the add_defaults is what was the original intention (although I thought there was a use case that I can't think of now for doing add_templates differently).

So for instance:

add_default([1,2,3])
add_default([4,5,6])

Should yield => [6,5,4,3,2,1]

Where as:

add_template([1,2,3])
add_template([4,5,6])

Will yield => [3,2,1,6,5,4]


- Kevin


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


On Sept. 26, 2013, 4 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, 4 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/134e5b5d/attachment-0001.html>