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

Fix incompatibility with isort configuration file #263

Merged
merged 1 commit into from Oct 22, 2019

Conversation

guewen
Copy link
Member

@guewen guewen commented Oct 16, 2019

The checker uses 'isort' to know if an imported library is in the
standard library or is a third party library. Then, for third party
libraries, the check will fail if it is not declared in the addon
manifest's external dependencies.

Some python modules are known to be used by Odoo and do not need to
be declared in the external dependencies. These libraries are listed
in a whitelist of the checker.

isort's SortImports class is initialized with an argument
'known_standard_library' where the whitelist is passed. It forces
isort to consider them as standard library, thus ignoring the check.

However, when the project's directory contains an '.isort.cfg' file,
because the project uses isort to actually sort the imports in the code,
the file usually contains a line such as:

[settings]
known_third_party = dateutil,openupgradelib,pkg_resources,psycopg2,requests,setuptools,urllib2,yaml

This line is necessary for isort to move the imports at the expected
place, after the standard imports.

When this file is present, the isort's 'place_module()' method will
consider it when called by the checker, and will consider the
whitelisted modules to be third party instead of the "expected" standard
library.

This commit directly checks if the module is in the whitelist, so we
avoid the confusion. isort is now only used to distinguish the standard
and third party which are not in the whitelist.

Fixes #247

The checker uses 'isort' to know if an imported library is in the
standard library or is a third party library. Then, for third party
libraries, the check will fail if it is not declared in the addon
manifest's external dependencies.

Some python modules are known to be used by Odoo and do not need to
be declared in the external dependencies. These libraries are listed
in a whitelist of the checker.

isort's SortImports class is initialized with an argument
'known_standard_library' where the whitelist is passed. It forces
isort to consider them as standard library, thus ignoring the check.

However, when the project's directory contains an '.isort.cfg' file,
because the project uses isort to actually sort the imports in the code,
the file usually contains a line such as:

[settings]
known_third_party = dateutil,openupgradelib,pkg_resources,psycopg2,requests,setuptools,urllib2,yaml

This line is necessary for isort to move the imports at the expected
place, after the standard imports.

When this file is present, the isort's 'place_module()' method will
consider it when called by the checker, and will consider the
whitelisted modules to be third party instead of the "expected" standard
library.

This commit directly checks if the module is in the whitelist, so we
avoid the confusion. isort is now only used to distinguish the standard
and third party which are not in the whitelist.

Fixes OCA#247
@sbidoul
Copy link
Member

sbidoul commented Oct 16, 2019

@guewen BTW, do you know that Odoo 13 now allows referencing PyPI distribution names in python depends rather than the import name? It's the recommended way to do it.

Sorry I don't have time to review this PR right now, I don't know if this changes something or not.

@guewen
Copy link
Member Author

guewen commented Oct 16, 2019

Yes, and this checker doesn't take this into account. A similar check than odoo/odoo#25549 should be added in

if module_name not in py_ext_deps and \
module_name.split('.')[0] not in py_ext_deps:

Should be the matter of another PR

Copy link
Collaborator

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

👍

@pedrobaeza pedrobaeza merged commit cb8b53b into OCA:master Oct 22, 2019
guewen added a commit to guewen/queue that referenced this pull request Oct 30, 2019
This is a temporary change to the configuration before
OCA/pylint-odoo#263 is included in a release.
The risk of having a new dependency in the queue addons is lower
than excluding every addons from pre-commit.
yelizariev added a commit to yelizariev/itpp-runbot that referenced this pull request May 14, 2020
Specifically, it fixes false negative result for
W7936(missing-manifest-dependency)
See OCA/pylint-odoo#263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Third party libraries such as psycopg2 marked as W7936(missing-manifest-dependency)
7 participants