Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Croatia cities and states #2

Merged
merged 1 commit into from
Apr 27, 2018
Merged

Add Croatia cities and states #2

merged 1 commit into from
Apr 27, 2018

Conversation

badbole
Copy link
Contributor

@badbole badbole commented Apr 24, 2018

No description provided.

Izvor:

Croatian counties (without HR- prefix in code) https://en.wikipedia.org/wiki/ISO_3166-2:HR
Hr Pošta webstranice
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is relevant for the description to be also in english. Could you add that please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, added these files, as i saw in spain localization, lots of readme files was only in spanish, so i thought for localizations descriptions are allowed to be only in local language,
from now on i will try to write all the explanations and readmes in english.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In localization repos, README can be in native language.

-->

<record id="state_hr_01" model="res.country.state">
<field name="country_id" ref="base.hr"/>
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add the Country?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1 @@
__import__('pkg_resources').declare_namespace(__name__)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not wrong, but there is no need to have the trouble to add these files: they will be automatically generated by an OCA bot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tnx for pointing out this part... wasn't sure if this will be automatic or not so i added it in first commit, and repeated in this one..
Will ommit this in future.

@pedrobaeza
Copy link
Member

I'm seeing that Geonames includes all these names: http://download.geonames.org/export/dump/HR.zip. You can use base_location_geonames_import module as base the same as we do with l10n_es_toponyms for avoiding to need this bunch of data.

@badbole
Copy link
Contributor Author

badbole commented Apr 25, 2018

@pedrobaeza tnx for pointing geonames...
last time i checked it was a bit outdated, regarding obsolete post numbers etc...
will check again, and possibly propose update to geonames db..
please allow few days for check and data compare
have to make sure that data is future compatabile with other localisation modules i plan to commit

@pedrobaeza
Copy link
Member

In l10n_es_toponyms, we transform state codes to be compatible with the rest of our localizations modules, so you can take that as an inspiration. Our case is a bit special though, as Odoo core includes Spanish state information since v10, but it's similar.

@badbole
Copy link
Contributor Author

badbole commented Apr 25, 2018

my reason for makin this module, is this way we have imported data with proper ids, that can be called as reference in other modules that adds some other specific coding, so i assumed easier to reference on static triple checked data end extend them using thopse defined ids. also, I saw a lot of simmilar modules in other localizations, (swiss, germany, argentina, france) addins states, and some of them add cities... also i'm aware that these modules are ported from older versions, before base_geonames_import module was introduced...
Anyway.. i see no harm in having/keeping this module, but will also double check your proposied solution ..

@badbole
Copy link
Contributor Author

badbole commented Apr 26, 2018

@pedrobaeza i have done some testig as you proposed.
importing Croatia cities from geonames database results in some 6900+ city entries,
biggest problem with data inaccuracy when importing from geonames is :
all cities imported from geonames belong to one state (local name:Županija) : Zagrebačka.
(wich is totaly incorrect - maybe one day i find some spare time and propose correction for this in geonames database)
proposed l10n_hr_base_location module has only 869 entries, which sattysfy most business needs.
and all data is correct regarding city-state relation
I also tested usage of geonames import after installing l10n_hr_base - possible, all correct states assigned to cities by l10n_hr_base are overwritten and city state relation is non sattisfying.
Instaling l10n_hr_base_location after geonames import, results in on correcty imported data, but data not imported from module is still not good regarding city-state relations.

Conclusion: I would still like to merge this pull request, based on these arguments, data from module is minimized and triple checked to be compliant with post current codes.
do i have green for merge?

Copy link
Sponsor Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, since geonames data is not accurate.

@pedrobaeza
Copy link
Member

OK for me too. Proceed with the merge.

@badbole
Copy link
Contributor Author

badbole commented Apr 27, 2018

excelent! tnx for help, merging this one.

@badbole badbole merged commit 1d310b8 into OCA:10.0 Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants