> On Aug. 19, 2014, 6:05 p.m., rmudgett wrote: > > branches/1.8/tests/test_callerid.c, line 608 > > <https://reviewboard.asterisk.org/r/3923/diff/1/?file=66615#file66615line608> > > > > 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().
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. > On Aug. 19, 2014, 6:05 p.m., rmudgett wrote: > > branches/1.8/main/callerid.c, line 1111 > > <https://reviewboard.asterisk.org/r/3923/diff/1/?file=66611#file66611line1111> > > > > 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. > > opticron wrote: > 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. 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. - Mark ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3923/#review13120 ----------------------------------------------------------- On Aug. 19, 2014, 1:55 a.m., opticron wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3923/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2014, 1:55 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > 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. > > > Diffs > ----- > > branches/1.8/tests/test_callerid.c PRE-CREATION > branches/1.8/res/res_agi.c 421326 > branches/1.8/main/privacy.c 421326 > branches/1.8/main/manager.c 421326 > branches/1.8/main/callerid.c 421326 > branches/1.8/include/asterisk/callerid.h 421326 > branches/1.8/channels/chan_unistim.c 421326 > branches/1.8/channels/chan_misdn.c 421326 > branches/1.8/apps/app_voicemail.c 421326 > branches/1.8/apps/app_dial.c 421326 > > Diff: https://reviewboard.asterisk.org/r/3923/diff/ > > > Testing > ------- > > Ran the unittest. > > > Thanks, > > opticron > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
