Code review comment for lp:~rachidbm/ubuntu-l10n-tools/search

On Mon, May 30, 2011 at 6:35 PM, David Planella
<email address hidden>wrote:

> A couple of other things I've noticed (I'm still reviewing the 'search
> both' implementation):
>
> * On ul10n_tools/search/TranslationsSearch.py, line 336 of the diff, please
> don't reformat the code. In general, when submitting merge proposals the
> changes should concentrate on the feature, so that the diff contains as
> little noise as possible.
>
I understand that this is not desirable for the reviewer (and now bazaar
thinks I changed that code). I'll try to check the diff before committing.
Tips on this procedure are welcome.

> * On ul10n_tools/search/TranslationsSearch.py, line 333, one thing that I
> really like is that you've managed to unify the code to have the same search
> loop for plural and singular entries (the "for entry_to_search in
> entries_to_search:" part. I think it might be interesting to explore
> optimizing this further and rewrite the for loop in a generic way that would
> work for both plural and singular, and have it called just once after the if
> clause in which we detect if the entry is plural or singular.
>
Thanks and good point. I think it can be done relatively easy by changing
the __add_match()
Just give it the entire entry object to add_match and do the check for
plural there (if that's still needed though).
By looking to the code I think that just giving the entry to __add_match is
enough. But I'll find that out.

>
> What do you think?
> --
> https://code.launchpad.net/~rachidbm/ubuntu-l10n-tools/search/+merge/62857
> You are the owner of lp:~rachidbm/ubuntu-l10n-tools/search.
>