<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2846/">https://reviewboard.asterisk.org/r/2846/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 30th, 2013, 4:19 p.m. UTC, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2846/diff/2/?file=46653#file46653line40" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">def</span> <span class="nf">get</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> <span class="n">key</span><span class="p">,</span> <span class="n">from_self</span><span class="o">=</span><span class="bp">True</span><span class="p">,</span> <span class="n">from_templates</span><span class="o">=</span><span class="bp">True</span><span class="p">,</span> <span class="n">from_defaults</span><span class="o">=</span><span class="bp">True</span><span class="p">):</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">41</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="n">from_self</span> <span class="ow">and</span> <span class="n">key</span> <span class="ow">in</span> <span class="bp">self</span><span class="p">:</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</blockquote>
<p>On October 14th, 2013, 6:50 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On October 14th, 2013, 10:10 p.m. UTC, <b>Kevin Harwell</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Got it. Drop this finding.</pre>
<br />
<p>- Matt</p>
<br />
<p>On September 26th, 2013, 9 p.m. UTC, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Sept. 26, 2013, 9 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22374">ASTERISK-22374</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested against various sip.conf files to ensure that generated sections had the expected data in them.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/contrib/scripts/sip_to_pjsip/astdicts.py <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/contrib/scripts/sip_to_pjsip/astconfigparser.py <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/contrib/scripts/sip_to_pjsip/sip_to_pjsip.py <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/contrib/scripts/sip_to_res_sip/astconfigparser.py <span style="color: grey">(399909)</span></li>
<li>/trunk/contrib/scripts/sip_to_res_sip/astdicts.py <span style="color: grey">(399909)</span></li>
<li>/trunk/contrib/scripts/sip_to_res_sip/sip_to_res_sip.py <span style="color: grey">(399909)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2846/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>