https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15541
--- Comment #12 from David Cook <[email protected]> --- Thanks for your comments, Marcel! I appreciate you taking the time to look at the patch. (In reply to Marcel de Rooy from comment #11) > QA Comment: > Thanks for your patch, David. > BTW This is imo not a trivial patch or something to signoff for an academy > user, new to Koha. Fair enough. I think perhaps the patch grew with time and I forgot to remove the Academy keyword. > [1] Although your changes in SimpleSearch are very small, it would probably > help if you would add a test case in t/db_dependent/Search.t. (It seems to > fail some tests currently, separate from your patch.) Hmm, also fair enough. I'll look at doing that. > [2] The source_normalizer or norms just lead me to two FIXMEs: > # FIXME normalize, substr > # FIXME - default normalizer > You introduce none and raw in the regex while this field actually does > nothing (and should not have been there in this stage). Your patch starts > using this field, while the only thing you want is not running _normalize > for (mainly?) URLs. If we do not really solve the problem of this unused > field, I think we should not touch it. BTW What would be the difference > between raw and none? The regex suggests more than what we offer. I'm not sure what you mean here... so I'll look at the code again. I think Kyle was having problems with normalization, so it's possible that he removed _normalize() in a different patch? > [3] I have some doubts about this new condition: > if ($QParser && $matchpoint->{'index'} !~ m/\w,\w/) { > Somehow you managed to bump in another unfinished area here too :) > I would rather leave the condition as it was. In the QParser branch you > could choose to ignore/remove the second specifier word with a similar regex. > Hmm, I don't think that's a good idea. You don't want to ignore/remove the second qualifier; it's important that the qualifier is in the query. You want to use it, but I don't think the QParser can do it. I think I lifted this condition from a different part of the Koha code to make the behaviour a bit more consistent. Perhaps it would be smarter to add a more universal "can_query_parse()" or something to test if the query is parseable by QParser... > Changing status for now. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
