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

Prevent ImportError by default. #224

Closed
wants to merge 1 commit into from

Conversation

yajo
Copy link
Member

@yajo yajo commented Sep 23, 2016

Fixes / new features

Tired of fixing ImportErrors again and again and again and again...

@Tecnativa

@sbidoul
Copy link
Member

sbidoul commented Sep 23, 2016

I'm not favorable to hide real errors. 👎

@yajo
Copy link
Member Author

yajo commented Sep 23, 2016

What am I hiding? The exc_info thing logs the full exception, and the INFO log is explicit enough to let you know something bad happened.

OTOH, ImportError can only be raised when importing, any other exception is not caught.

@sbidoul
Copy link
Member

sbidoul commented Sep 23, 2016

You are transforming potential real bugs (eg a mistyped import statement) into info/debug level logs.

If you want to solve spurious CI errors due to missing dependencies, let's work on the root cause instead: OCA/maintainer-quality-tools#343 (BTW, I'm willing to work on this during the october sprint)

@danimaribeiro
Copy link

What's the purpose of loading a module in a inconsistent state?
👎

@lmignon
Copy link
Contributor

lmignon commented Sep 23, 2016

👎

@pedrobaeza
Copy link
Member

You're mixing things: you're not loading modules (they're not even installed), it's Odoo who loads them, at least the Python part. With this, you prevent your server stalls at loading. And it happens when you add a repository to your addons_path for using one of the modules, but you don't want to add the corresponding dependencies for non-installed modules.

Second: I see good all the pip promotion, but there's still a lot of people who don't want or can't use this method, so we shouldn't make this fail in that cases.

@pedrobaeza
Copy link
Member

Said that, we can lower the try at a deep level for not being so "aggressive" and this masks other programming errors.

@lmignon
Copy link
Contributor

lmignon commented Sep 23, 2016

@pedrobaeza I strongly disagree. The proposed change is not for libs outside the scope of the addons but for sub modules of the current addons. The guidelines already propose a workaround for the case you describe.

@moylop260
Copy link
Contributor

@yajo
I want understand your point first.

Do you have a real case showing a ImportError with from . import something?

@moylop260
Copy link
Contributor

I understand your point (I saw the url of again)

FYI we have a guideline for external dependencies
https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#external-dependencies

You could use it to add this case for template module.

@moylop260
Copy link
Contributor

Maybe we can create a new pylint check to validate this case.
Whitelist of allowed imports (internal libraries).
If import not in whitelist then check if there is a try-except-ImportError-logger

@yajo
Copy link
Member Author

yajo commented Sep 23, 2016

I understand the downvotes, but certainly we need a solution for #224 (comment). @moylop260's suggestion would be better indeed, however I'm not sure on how to do that...

About pip... I'm not sure on how the pip installing works, but I feel it would harden the task of merging PRs on your production environments, which is something we basically cannot live without. @sbidoul Is there a way to combine both of those needs?

@sbidoul
Copy link
Member

sbidoul commented Sep 23, 2016

@yajo

I feel it would harden the task of merging PRs on your production environments

Merging PR in real world deployment is absolutely necessary indeed. Pip does not make it harder, on the contrary:

  • pip has a develop/editable mode to install unreleased code (pip install -e repo/setup/addon_name)
  • there is git-aggregator to manage merges in a repeatable manner

It's true it's a change of habitude and it require some learning, but we are very happy we did this change at acsone after several years using buildout. The good thing is that what you learn with pip is applicable almost everywhere python is used.

@moylop260
Copy link
Contributor

FYI I have created the check for pylint-odoo: OCA/pylint-odoo#68

@yajo
Copy link
Member Author

yajo commented Sep 24, 2016

Thanks to both! Closing in favour of the pylint alternative

@yajo yajo closed this Sep 24, 2016
@yajo yajo deleted the master-protect-imports branch September 24, 2016 07:42
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.

6 participants