Review: Needs Information Functionally it looks good to me, with one exception: despite name_get() not using the `zip` column, it seems a useful thing to allow searching on zipcodes, as it could be a useful way to get a shortlist of possible candidates. Seeing that it used to have the priority over other columns in the previous implementation, dropping it completely sounds dangerous for a stable version. We could simply add it to the default list of fields, this still means that name_search will behave as expected wrt name_get (especially since zip codes can hardly be confused with other column values).
On a technical level I have one performance question: have you tested this name_search() implementation on a large database containing 10k+ randomly distributed addresses? Past experience told us that performance can become terrible when searching on an OR'ed domain involving translatable records (such as countries here!), due to the complexity of the query being executed on the database. For instance for product.name_search() we had to specifically execute the various sub-queries separately and merge the results in-memory, otherwise the system became very slow with 10k products [1]. I think it makes sense to double-check this here too - creating 10k random addresses is easy with a few `INSERT SELECT` queries. Finally, some minor technical remarks: - l.10,l.23: while you're changing the code, it would be nice to use proper spacing for == operators ;-) - l.25: using a list comprehension is usually much more readable than map(): [(f, operator, name) for f in fields] vs map(lambda f: (f, operator, name), fields) - l.25: using expression.OR(domains...) might be more readable than hand-crafting the OR'ed domain Thanks! [1] See the product.name_search() implementation note: http://bazaar.launchpad.net/~openerp/openobject-addons/6.1/view/head:/product/product.py#L619 -- https://code.launchpad.net/~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco/+merge/98196 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
