Feature: Using custom ConnectorEnvironment #108

Merged
merged 6 commits into from Aug 21, 2015

Conversation

Projects
None yet
4 participants
@maljac
Member

maljac commented Aug 20, 2015

Related to #107

@lmignon

This comment has been minimized.

Show comment
Hide comment
@lmignon

lmignon Aug 20, 2015

Contributor

@maljac Thank you for the contribution. Can you provide some unit tests to stress the new functionality and fix the pep8 issue?

======== Testing test_flake8 ========
./connector/connector.py:217:21: E126 continuation line over-indented for hanging indent
Contributor

lmignon commented Aug 20, 2015

@maljac Thank you for the contribution. Can you provide some unit tests to stress the new functionality and fix the pep8 issue?

======== Testing test_flake8 ========
./connector/connector.py:217:21: E126 continuation line over-indented for hanging indent
@@ -316,6 +326,29 @@ def get_connector_unit(self, base_class):
return self.backend.get_class(base_class, self.session,
self.model_name)(self)
+ @classmethod
+ def create_environment(cls, backend_record, session, model,
+ connector_env=None):

This comment has been minimized.

@guewen

guewen Aug 20, 2015

Member

When a connector_env is given, we already know the backend_record, the session and the model (which may change) so we can wonder if they could not all be optional (but mandatory when we have no connector_env). Still, thinking about it, it would make the body of the class method and the understanding of the API more complicated so it maybe doesn't worth the gain of removing the repetition.

@guewen

guewen Aug 20, 2015

Member

When a connector_env is given, we already know the backend_record, the session and the model (which may change) so we can wonder if they could not all be optional (but mandatory when we have no connector_env). Still, thinking about it, it would make the body of the class method and the understanding of the API more complicated so it maybe doesn't worth the gain of removing the repetition.

This comment has been minimized.

@maljac

maljac Aug 20, 2015

Member

Yeah, I was thinking about the same. But "explicit is better than implicit", so as you said, the API would be more complicated.

@maljac

maljac Aug 20, 2015

Member

Yeah, I was thinking about the same. But "explicit is better than implicit", so as you said, the API would be more complicated.

maljac added some commits Aug 20, 2015

connector/connector.py
+
+ List of attributes that must be used by
+ :py:class:`connector.connector.ConnectorEnvironment.create_environment`
+ when a new connector environment is instiated.

This comment has been minimized.

@guewen

guewen Aug 20, 2015

Member

small typo s/instiated/instantiated/

@guewen

guewen Aug 20, 2015

Member

small typo s/instiated/instantiated/

connector/connector.py
+ :type session: :py:class:`connector.session.ConnectorSession`
+ :param model_name: name of the model
+ :type model_name: str
+ :param connector_env:

This comment has been minimized.

@guewen

guewen Aug 20, 2015

Member

Suggestion: an existing environment from which the kwargs will be propagated to the new one
Feel free to explain it in another way or improve my suggestion

@guewen

guewen Aug 20, 2015

Member

Suggestion: an existing environment from which the kwargs will be propagated to the new one
Feel free to explain it in another way or improve my suggestion

@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Aug 20, 2015

Member

Thanks for this excellent and well realized contribution!
Before a merge, could you add yourself in the authors file (https://github.com/OCA/connector/blob/8.0/connector/AUTHORS) and add a line describing your change in the changelog (https://github.com/OCA/connector/blob/8.0/connector/CHANGES.rst in the "Future" section)?

Member

guewen commented Aug 20, 2015

Thanks for this excellent and well realized contribution!
Before a merge, could you add yourself in the authors file (https://github.com/OCA/connector/blob/8.0/connector/AUTHORS) and add a line describing your change in the changelog (https://github.com/OCA/connector/blob/8.0/connector/CHANGES.rst in the "Future" section)?

@lmignon

This comment has been minimized.

Show comment
Hide comment
@lmignon

lmignon Aug 20, 2015

Contributor

Thank you @maljac 👍

Contributor

lmignon commented Aug 20, 2015

Thank you @maljac 👍

@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Aug 20, 2015

Member

I propose you to amend your last commit and to remove the (tiny change) on your name ;-)
Thanks 👍

Member

guewen commented Aug 20, 2015

I propose you to amend your last commit and to remove the (tiny change) on your name ;-)
Thanks 👍

@maljac

This comment has been minimized.

Show comment
Hide comment
@maljac

maljac Aug 20, 2015

Member

Since you are the project lead, I leave the decission up to you to decide what is a tiny change or not ;-)
commit amended

Member

maljac commented Aug 20, 2015

Since you are the project lead, I leave the decission up to you to decide what is a tiny change or not ;-)
commit amended

@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Aug 20, 2015

Member

The one who makes a complete PR with docstring and tests can't be granted a tiny change ;-)

We just have to wait some days or have a third approval before we can merge according to the OCA's rules.

Member

guewen commented Aug 20, 2015

The one who makes a complete PR with docstring and tests can't be granted a tiny change ;-)

We just have to wait some days or have a third approval before we can merge according to the OCA's rules.

connector/connector.py
+ .. attribute:: _propagate_kwargs
+
+ List of attributes that must be used by
+ :py:class:`connector.connector.ConnectorEnvironment.create_environment`

This comment has been minimized.

@guewen

guewen Aug 20, 2015

Member

this one should be :py:meth:

@guewen

guewen Aug 20, 2015

Member

this one should be :py:meth:

connector/CHANGES.rst
@@ -6,6 +6,7 @@ Future
* method 'install_in_connector' is now deprecated
* Add a retry pattern for jobs (https://github.com/OCA/connector/pull/75)
+* Use custom connector environments and instatiate them with needed attributes (https://github.com/OCA/connector/pull/108)

This comment has been minimized.

@guewen

guewen Aug 20, 2015

Member

s/instatiate/instanciate/

@guewen

guewen Aug 20, 2015

Member

s/instatiate/instanciate/

@mdietrichc2c

This comment has been minimized.

Show comment
Hide comment
@mdietrichc2c

mdietrichc2c Aug 21, 2015

Contributor

👍 on this great PR

Contributor

mdietrichc2c commented Aug 21, 2015

👍 on this great PR

guewen added a commit that referenced this pull request Aug 21, 2015

Merge pull request #108 from maljac/8.0_feature_custom_connector_envi…
…ronment

Feature: Using custom ConnectorEnvironment

@guewen guewen merged commit 66b9fb4 into OCA:8.0 Aug 21, 2015

2 checks passed

ci/runbot runbot build 3113428-108-32bd67 (runtime 31s)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Aug 21, 2015

Member

Thanks for your work!

Member

guewen commented Aug 21, 2015

Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment