[REF] pylint.cfg: Enable missing-import-error, missing-manifest-dependency

[moylop260]

Enable new checks from: OCA/pylint-odoo#68
Version 1.3.3

[moylop260]

@yajo @bealdav @alan196 your feedback is welcome

[alan196]

👍

[moylop260]

ping

[jjscarafia]

👍

[moylop260]

@pedrobaeza
Could you merge it, please?

[pedrobaeza]

This checks should be mandatory, not optional for PRs, as these are critical errors, so I think you should require them on 8.0/9.0 (and the upcoming 10.0).

[moylop260]

Fixed

[yajo]

Conflicts found

[moylop260]

Sorry for delay...
Rebased

[pedrobaeza]

Coverage is dropping a lot. Can you check it?

[moylop260]

Coveralls is failing https://coveralls.io/builds/8278931/source?filename=travis%2Fself_tests again
Codecov is green.

[pedrobaeza]

OK, I see, merging

[yajo]

[lmignon]

I hate this kind of new check! Why do I have to protect my import in connector-cmis? https://travis-ci.org/OCA/connector-cmis/jobs/189107369 This project has only one addon (9.0 and 10) and didn't work without the external dependency!!!!!! I am tired of these changes that break the tests by asking once again to adopt new rules decided by a minority.

[moylop260]

@lmignon
I dont understand your real point.

Do you have a proposal to add a exception or change the guideline https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#importerror?

[moylop260]

Or Do you have a proposal to change the workflow of approves for guidelines?

[lmignon]

@moylop260 In the case of the connector-cmis it's a non sens to put the project in your addon path if you don't use it and if you use it you must have installed the external dependency. Therefore why do I have to protect the import in my source code.... The rule is not applicable to small repository build around an external library. MOREOVER IMO all changes in MQT rules should be approved by a vote of the OCA delagates!

[pedrobaeza]

I don't agree: you must always protect your external imports. It's a way to avoid an installation break if you have these modules in your addons path. In an ideal world, this shouldn't be needed, but it's not an ideal one. And think in other people that don't use pip, and Odoo's bug about static folder...

[moylop260]

@lmignon If you have a good reason to skip this check just for that case you could use a # pylint: disable=CHECK-NAME and add the comment above.

If you think that is a global exception you could create a new PR for guidelines to change it. (And I change the lint).

EXTRA NOTE: Maybe the main problem is have a project with just a module. (I don't know).

MOREOVER IMO all changes in MQT rules should be approved by a vote of the OCA delagates!

Like this PR?

[lmignon]

@pedrobaeza don't put connector-cmis in your addon path if you don't use it... This rule IS STUPID in the case of small project build around an external lib! That's the case of connector-cmis.

Maybe it's time I leave OCA...

[moylop260]

@lmignon
I'm trying help you to detect your exception, please, be patient I am not so smart like you but I try help.

This rule IS STUPID in the case of small project build around an external lib!
I don't know how to detect a small project from the lint but ideas are welcome?

Do pylint disable comment looks fine for you for this cases?

[pedrobaeza]

@lmignon if you think OCA doesn't benefit you, then just leave it, but I don't think it's the right idea. These conventions are built around a past knowledge of troubles that we try to avoid in the future. That's why a guard in the import, that are only 3 new lines, assures a correct functioning in all the environments. We have tried to correct this in the root (Odoo not loading python files when there is a static folder, but without no success). If we get it in the future, this convention/check just can go out, but for now it's not possible.

[lmignon]

@moylop260 Indeed it's not possible or very difficult to automatically detect a small project build around an external lib. Is it possible to disable a rule at repository level so the PSC can decide to relax a specific rule according to the nature of the project?

[moylop260]

relax a specific rule according to the nature of the project?

That is a good point.
Thanks for sharing.

I'll check it

[lmignon]

@moylop260 Thank you for your listening.

[yajo]

Hi @lmignon.

Please review OCA/server-tools#89, OCA/openupgradelib#21, OCA/server-tools#232, OCA/geospatial#3, OCA/account-financial-reporting#229, OCA/l10n-spain#255, OCA/reporting-engine#64, and OCA/reporting-engine#62, just to name a few.

Specifically OCA/server-tools#666 recently broke all website builds. This means lots of headaches trying to find what in the world happened to those builds that were perfectly green yesterday, fixing it, opening PR, waiting for merge, rebasing every open website PR, and hoping all goes well and nobody else did a import without try-except that breaks a different addon and you have to start again. All of that should have never happened if the original developer acknowledged this simple linter error, such as any other you might fix.

Since the OCA is composed of a combination of many small teams that makes a big community, developing in a way that does not break others' deployments increases others' respect for you. The opposite is also true.

If we put the community effort in a balance, fixing this linter error is a matter of minutes at PR time, but a matter of hours when it is already merged. That's why there was this improvement to our tools, that should save hours at the cost of minutes. Community coding is this way, I hope you understand.

So, summarizing, just add that simple try-except and stop complaining, friend 😉

[lmignon]

or stop contributing

[sbidoul]

Emotional reactions aside, I still believe that peppering all the OCA code with import guards is a ugly, unpythonic hack. That hurts the eyes (at least mine 😉), and makes diagnosing real import issues harder. And, fundamentally, it does not address the root cause of the import issues.

There are several ways to address those:

• lobby Odoo SA to have a mechanism to not import uninstalled modules unless explicitly needed, eg [ADD] new manifest key no_autoload odoo/odoo#8591
• install only required dependencies in CI: Update travis scripts to install with pip #343
• install all dependencies mandated by a repo: if that becomes a burden, because there are heavy dependencies you don't need, it's probably a sign that said repo has become too big and must be split
• ... and maybe other approaches that I did not think of

[yajo]

I agree, mostly with odoo/odoo#8591

[lasley]

Also agreed on odoo/odoo#8591

I think it would also be nice for a repo-level pylint config allowing for disabling of lints for reasons other than this, but that feature is probably best started in a new issue I think.