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] l10n_fr_naf_ape from community-data-files #3

Merged
merged 14 commits into from
Mar 13, 2015

Conversation

clonedagain
Copy link

Module moved from the OCA repository community-data-files.
First step to fix #1 .
The module works as-is on v8. README file added.

@clonedagain clonedagain force-pushed the 8.0-add-naf-nace branch 2 times, most recently from 5028eb3 to 0824ce2 Compare December 16, 2014 10:03
@clonedagain
Copy link
Author

Updated to include the revision history of the 2 modules which I had neglected to include.

@yvaucher
Copy link
Member

👍

@legalsylvain
Copy link
Contributor

Hi,
Thanks for porting this module. 2 thinks :

  • It's better to make 2 PR for two modules;

  • For l10n_eu_nace, I don't see the reason to include an european module into fr localization;

    ❓ CC : @alexis-via

@yvaucher
Copy link
Member

@legalsylvain Do you suggest to add an european repository?

@legalsylvain
Copy link
Contributor

In fact, if it is a EU project, it will be usefull for german, italian teams.
So, I would like to avoid forking that module into all eu localization.
Other solution is to let this module into community-data-files, it isn't ?
What do you think ?
CC : OCA: @sebastienbeau, @gurneyalex, @alexis-via ...

@clonedagain
Copy link
Author

The French part extends the EU part so I'd rather have both in the same
repo, otherwise the French part is not installable.
I have no idea what's best, opinions are welcome.

Le 18/12/2014 13:55, Sylvain LE GAL a écrit :

In fact, if it is a EU project, it will be usefull for german, italian
teams.
So, I would like to avoid forking that module into all eu localization.
Other solution is to let this module into community-data-files, it isn't ?
What do you think ?
CC : OCA: @sebastienbeau https://github.com/sebastienbeau,
@gurneyalex https://github.com/gurneyalex, @alexis-via
https://github.com/alexis-via ...


Reply to this email directly or view it on GitHub
#3 (comment).

alexis-via pushed a commit that referenced this pull request Jan 26, 2015
FIX declaration of external deps on l10n_fr_base_location_geonames_import
{
"name": "European NACE partner categories",
"version": "2.0",
'author': u'Numérigraphe SARL, Sistheo',
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add "Odoo Community Association (OCA)" to the list of authors?

@gurneyalex
Copy link
Member

maybe we could use https://github.com/OCA/community-data-files for the l10n_eu_nace addon?

@pedrobaeza
Copy link
Member

Isn't already there but unported (https://github.com/OCA/community-data-files/tree/8.0/__unported__/l10n_eu_nace)? I think that one should be "OCA level", not localization level.

@clonedagain
Copy link
Author

Thanks all, I'll update this proposal based on your suggestions.

l10n_eu_nace can live on in community-data-files if it fits more there. I'll add a warning (EDIT: in the description) so interested users can install it.

clonedagain pushed a commit to numerigraphe/community-data-files that referenced this pull request Mar 6, 2015
…d3b54b6'

git-subtree-dir: l10n_fr_naf_ape
git-subtree-mainline: 840b586
git-subtree-split: 09c25ac
@clonedagain clonedagain force-pushed the 8.0-add-naf-nace branch 2 times, most recently from b74f7ae to 34bfa65 Compare March 6, 2015 10:45
@clonedagain clonedagain changed the title [ADD] l10n_fr_naf_ape, l10n_eu_nace [ADD] l10n_fr_naf_ape from community-data-files Mar 6, 2015
@clonedagain
Copy link
Author

Done. I've also tested the modules on v8: they work unchanged.

Travis fails here because the community-data-files part is __unported__.
Can someone please review OCA/community-data-files#3 ?

@gurneyalex gurneyalex added 8.0 and removed question labels Mar 9, 2015
To install this module, you may either:

* download it from the Odoo app store: https://www.odoo.com/apps?search=l10n_eu_nace
* OR download and install the code of the OCA project *Community data files*: https://github.com/numerigraphe/community-data-files
Copy link
Member

Choose a reason for hiding this comment

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

@gurneyalex
Copy link
Member

overall 👍 once points above have been resolved

@legalsylvain
Copy link
Contributor

Thanks all, I'll update this proposal based on your suggestions.
l10n_eu_nace can live on in community-data-files if it fits more there. I'll add a warning (EDIT: in the description) so interested users can install it.

@clonedagain : Just my PoV, but I think the warning is unecessary. All dependencies are written in Travis File. So information is redundant. It should be usefull if it was a non OCA repository.

@legalsylvain
Copy link
Contributor

👍 Code review. No test.
Thanks for porting this module and for the refactoring into to various repositories.

@yvaucher
Copy link
Member

yvaucher commented Mar 9, 2015

needs fixing as some remarks from @gurneyalex still needs to be addressed

@clonedagain
Copy link
Author

Fixed I think, Travis should succeed once OCA/community-data-files#3 gets merged.

@clonedagain
Copy link
Author

Build passes if you force it: https://travis-ci.org/OCA/l10n-france/builds/54259385
Can we merge please ?

@yvaucher
Copy link
Member

Travis restarted

yvaucher added a commit that referenced this pull request Mar 13, 2015
[ADD] l10n_fr_naf_ape from community-data-files
@yvaucher yvaucher merged commit f30cddb into OCA:8.0 Mar 13, 2015
@yvaucher
Copy link
Member

Ok merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants