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

9.0 port partner_external_maps #237

Merged
merged 10 commits into from
Apr 5, 2016
Merged

Conversation

alexis-via
Copy link
Contributor

Contributed by Wim Audenaert.

@alexis-via
Copy link
Contributor Author

Travis is red because "Transifex password not recognized- exiting early." ; all the other travis tests are green.

@yvaucher yvaucher added this to the 9.0 milestone Jan 13, 2016
@pedrobaeza
Copy link
Member

Please rebase

@alexis-via
Copy link
Contributor Author

Rebase done. The file REAME.md is really a pain, because it will always conflict everytime a new module is ported.

@pedrobaeza
Copy link
Member

Not if you don't change it. And you mustn't. This is done automatically by an script, so you should abstent to update manually.

@@ -41,7 +42,11 @@ addon | version | summary
[partner_auto_salesman](partner_auto_salesman/) | 8.0.1.0.0 (unported) | Partner auto salesman
[partner_contact_address_detailed](partner_contact_address_detailed/) | 8.0.1.0.0 (unported) | All address data in summarized contact form
[partner_contact_birthdate](partner_contact_birthdate/) | 8.0.1.0.0 (unported) | Contact's birthdate
<<<<<<< 3398e80890c1e14aa3131baad1a5bf6f9ed6a96b
Copy link
Member

Choose a reason for hiding this comment

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

This has not been correctly rebase. Please remove these changes. As stated, they are done automatically after the merge by an script.

@alexis-via
Copy link
Contributor Author

Could someone merge this please ?

@@ -23,7 +23,7 @@

{
'name': 'Partner External Maps',
'version': '8.0.0.1.0',
'version': '9.0.0.1.0',
Copy link
Member

Choose a reason for hiding this comment

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

9.0.1.0.0

@pedrobaeza
Copy link
Member

Some things that I consider before merging:

@elicoidal
Copy link

Can you please update the copyright headers to the simple version in all py files following OCA templates
https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L3

@alexis-via
Copy link
Contributor Author

I think changing the name of the module is a bad idea in most cases (for me, the only good reason to change the name of the module would be if the destination/purpose of the module is radically changed). As the module is kept unchanged, I think we should keep the old name.

@pedrobaeza
Copy link
Member

We don't have to extend a firstly badly named module across versions, so we always take the migration as an occasion to change it. Please reconsider your position.

@alexis-via
Copy link
Contributor Author

Well, I don't think that the name is that bad... And if the purpose is only to remove the "s" at the end because some OCA board member had the brillant idea to invent a new "rule" that module names should not have an "s" at the end, then give him some "real" work (porting module to v9 for example) so that he can make himself helpful to the community. In the OCA, we need more guys doing the real work and less people inventing theorical "rules" about every details of modules.

@pedrobaeza
Copy link
Member

Hey, the change is not only about the s (I'm proposing partner_map), but this is not only a brilliant of an OCA board member (that's to say, me), but a general convention rule to avoid misspelling problems on every place where the module have to be named (XML-IDs, scripts, JS references...). You can find it here in the contributing conventions: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#modules (the first one in this section).

If the problem is the job of renaming, then I can do it.

@alexis-via
Copy link
Contributor Author

In fact, I am in favor of naming conventions, including for module names, but I think that these conventions should apply on new modules only. Changing the name of modules is a pain for users, in particular for non-expert users, for no real benefit.

But OK, if you really want to change the name of this module, you can do it.

@pedrobaeza
Copy link
Member

Name change and other improvements in akretion#2

@pedrobaeza
Copy link
Member

Sorry about the lint errors. May I do another PR for fixing them?

partner_external_map/models/map_website.py:6:1: F401 '_' imported but unused
partner_external_map/models/map_website.py:6:1: F401 'api' imported but unused
partner_external_map/models/res_partner.py:6:1: F401 'fields' imported but unused
partner_external_map/models/res_users.py:6:1: F401 '_' imported but unused

@alexis-via
Copy link
Contributor Author

i fixed them

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 97.416% when pulling c65139f on akretion:9-port-external-maps into 3398e80 on OCA:9.0.

@pedrobaeza
Copy link
Member

Thanks 👍

@alexis-via
Copy link
Contributor Author

Ready to merge 👍

@lmignon
Copy link
Sponsor Contributor

lmignon commented Apr 5, 2016

👍 (Code review + functional)

@pedrobaeza pedrobaeza merged commit 9c3ba09 into OCA:9.0 Apr 5, 2016
@pedrobaeza pedrobaeza mentioned this pull request Apr 5, 2016
31 tasks
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.

None yet

6 participants