<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/3923/">https://reviewboard.asterisk.org/r/3923/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 19th, 2014, 6:05 p.m. UTC, <b>rmudgett</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/3923/diff/1/?file=66611#file66611line1111" style="color: black; font-weight: bold; text-decoration: underline;">branches/1.8/main/callerid.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_isphonenumber(const char *n)</pre></td>
</tr>
</tbody>
<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">1111</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">escape_quotes</span><span class="p">(</span><span class="n">name_start</span><span class="p">,</span> <span class="n">quote_count</span><span class="p">,</span> <span class="o">*</span><span class="n">name</span><span class="p">,</span> <span class="n">escaped_len</span><span class="p">))</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 am wondering if ast_callerid_parse() should be escaping the quotes at all. The job of ast_callerid_parse() is to take the string and separate it into a name and number string. Escaping characters seems wrong. If anything, the routine should be unescaping character sequences. The consumers likely don't know how to handle escaped quotes or may need to escape characters differently.</pre>
</blockquote>
<p>On August 19th, 2014, 7:20 p.m. UTC, <b>opticron</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;">You make a good point. This is going to take a bit to rework since I need to identify consumers of callerid information and have them escape quotes where necessary.</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;">Remember also that depending on the context you're in, escaping quotes may need to be done in different ways. In URLs, quotes are escaped as %22, whereas in XML they're escaped as " . So I agree that a core function like this shouldn't be trying to escape quotation marks on its own.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 19th, 2014, 6:05 p.m. UTC, <b>rmudgett</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/3923/diff/1/?file=66615#file66615line608" style="color: black; font-weight: bold; text-decoration: underline;">branches/1.8/tests/test_callerid.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">608</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="kt">int</span> <span class="n">unload_module</span><span class="p">(</span><span class="kt">void</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;">Rather than a bunch of redundant tests checking minor variations in the string. How about one test that works through an array of input string variatons and the expected broken out strings:
struct expect {
char *orig;
char *name;
char *number;
};
struct expect array[] = {
{ "name <number>", "name", "number" },
{ "\"name\" <number>", "name", "number" },
{ "name", "name", NULL },
{ "1234", NULL, "1234" }
}
The last two entries above are also valid possibilities for the input string to ast_callerid_parse().</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;">Hm, I see that this has a check mark as being resolved, so I may be wasting my breath here. I want to weigh in and say that I like the idea of having separate tests rather than a single one. That way, if there is a regression, it's eaiser to see that cases A, B, F, and H pass while cases C, D, E, and G fail. They're redundant, yes, and it may be possible to factor out some test logic into common functions, but having them as separate tests is a positive thing if you ask me.
If you've already combined these into a single test, though, then nevermind, just leave it like it is.</pre>
<br />
<p>- Mark</p>
<br />
<p>On August 19th, 2014, 1:55 a.m. UTC, opticron 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By opticron.</div>
<p style="color: grey;"><i>Updated Aug. 19, 2014, 1:55 a.m.</i></p>
<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 allows the callerid parsing function to handle malformed input strings and strings containing escaped and unescaped double quotes. This also adds a unittest to cover many of the cases where the parsing algorithm previously failed.</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;">Ran the unittest.</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>branches/1.8/tests/test_callerid.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/1.8/res/res_agi.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/main/privacy.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/main/manager.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/main/callerid.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/include/asterisk/callerid.h <span style="color: grey">(421326)</span></li>
<li>branches/1.8/channels/chan_unistim.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/channels/chan_misdn.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/apps/app_voicemail.c <span style="color: grey">(421326)</span></li>
<li>branches/1.8/apps/app_dial.c <span style="color: grey">(421326)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3923/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>