Code review comment for lp:~akretion-team/partner-contact-management/base-location-geonames-import

Oups I missed that.

Actually, I do not agrees with you, but I won't argue too long on this as for me the branch organisation problem is more deep. Let's see what other think about it.

Some comments Belows:
Some PEP8 in manifest:

328 + _("The content of the file doesn't correspond to the "
329 + "selected country."))

I will add row value and country info it will be more easy for support.

A question with :
+ if res_request.status_code != requests.codes.ok:
349 + raise orm.except_orm(
350 + _('Error:'),
351 + _('Got an error %d when trying to download the file %s.')
352 + % (res_request.status_code, url))

It's been a long time since I used geonames, if I'm correct in case of wrong country it returns 404 that right. If it the case I'm ok with this else we may have to treat a 200 with a message

+ if bzip_ids_to_delete:
356 + bzip_obj.unlink(cr, uid, bzip_ids_to_delete, context=context)
357 + logger.info(
358 + '%d better zip entries deleted for country %s'
359 + % (len(bzip_ids_to_delete), wizard.country_id.name))

This is a quite a direct approach, I agree create, update, unactivate is quite not easy to put in place, but as other development can depends on base location model there should at least be a Big warning in manifest.

Also it would be a good idea to add a small lock to ensure atomicity during the import.
A query "for update no wait" a the begining of transaction would be nice.

Tests are also missing. Without depending on the geoname services having a local excrept to base the tests would be great.

Thanks for the contributions, keep up the good work.

Regards

Nicolas