-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[FIX] users_ldap_populate: Protect ldap library import #232
Conversation
try: | ||
from ldap.filter import filter_format | ||
except ImportError: | ||
_logger.debug('Can not `from ldap.filter import filter_format`.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_logger.warning
instead of debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being discussed in https://github.com/OCA/account-financial-tools/pull/281/files#r37519322
👍 |
If fine to me |
As this module depends of auth_ldap, and that module already has that external_dependencies defined, is not explicitly needed. |
👍 |
@pedrobaeza the problem of expect the external_dependency only in the parent module, when you click on "install" the screen will retreive an error mentioning the dependency, not the module itself, IMHO it is useful here, even if technically it is not necesary, but BTW you have my +1 we have a rule to follow, and the rule do not mention explicitally how manage the depednency itself. |
@pedrobaeza |
@pedrobaeza once a module do an explicit import of an external dependency you must declare the dependency into the manifest. It's required to avoid that if the decency is removed from the dependent module the current one will be broken. |
@lmignon IMHO By this case the main goal is done! |
@moylop260 the rule is explicit.
from ldap.filter import filter_format The import means that this module use an extra dependency. |
d12a418
to
279755b
Compare
As stated before in others PRs "Better explicit than implicit", so I have added the external_dependencies in the manifest. |
comments applied, then +1 valid, I merge. |
[FIX] users_ldap_populate: Protect ldap library import
[FIX] users_ldap_populate: Protect ldap library import
To avoid errors on dependent builds