-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
[ADD] New unused-module check #449
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from . import odoo_addons | ||
from . import vim_comment | ||
from . import custom_logging | ||
from . import import_checker | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
import re | ||
from typing import Union | ||
|
||
from astroid import Import, ImportFrom, Module | ||
from astroid.modutils import modpath_from_file | ||
from pylint.checkers.utils import only_required_for_messages | ||
from pylint.lint import PyLinter | ||
|
||
from .odoo_base_checker import OdooBaseChecker | ||
|
||
ODOO_MSGS = { | ||
# C->convention R->refactor W->warning E->error F->fatal | ||
"E8401": ( | ||
"This python module is not imported and has no effect", | ||
"unused-module", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO this lint name is not specific enough. What kind of module? Take into account there are Odoo modules and Python modules I'd say Odoo models instead, e.g. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially it was for odoo models only but according to the previous review by Moy I made it generic. By Python module I mean and think I am following the official specification which as far as I understand is any As of now this basically detects unused python files |
||
"", | ||
), | ||
} | ||
|
||
EXCLUDED = re.compile( | ||
"__manifest__.py$|__openerp__.py$|__init__.py$|tests/|migrations/|docs/|tests\\\\|migrations\\\\|docs\\\\" | ||
) | ||
|
||
|
||
class ImportChecker(OdooBaseChecker): | ||
msgs = ODOO_MSGS | ||
name = "odoolint" | ||
|
||
def __init__(self, linter: PyLinter): | ||
self.imports = set() | ||
self.modules = set() | ||
|
||
super().__init__(linter) | ||
|
||
@staticmethod | ||
def get_module_from_node(node, max_attempts: int = 15) -> Union[None, Module]: | ||
"""Obtain the module which contains the given node. | ||
|
||
:param node: Node that belongs to the module which wil be obtained. | ||
:param int max_attempts: Number of attempts that will be made to obtain the module. Nodes that are | ||
nested deeper than max_attempts won't be found. | ||
|
||
:returns: Module if found, otherwise None. | ||
""" | ||
for _attempt in range(max_attempts): | ||
if not getattr(node, "parent", False): | ||
return None | ||
|
||
if isinstance(node.parent, Module): | ||
return node.parent | ||
|
||
node = node.parent | ||
|
||
return None | ||
|
||
def store_imported_modules(self, node): | ||
"""Store all modules that are imported by 'from x import y' and 'import x,y,z' statements. | ||
|
||
Relative paths are taken into account for ImportFrom nodes. For example, the following statements | ||
would import the following modules (consider the file which contains these statements is module.models.partner): | ||
|
||
``from ..wizard import cap, hello # module.wizard, module.wizard.cap, module.wizard.hello`` | ||
|
||
``from ..utils.legacy import db # module.utils.legacy, module.utils.legacy.db`` | ||
|
||
``from . import sale_order # module.models.sale_order`` | ||
|
||
``import foo # module.models.foo`` | ||
""" | ||
module = ImportChecker.get_module_from_node(node) | ||
if not module or "tests" in module.name: | ||
return | ||
|
||
if isinstance(node, ImportFrom): | ||
level = node.level or 0 | ||
elif isinstance(node, Import): | ||
level = 1 | ||
else: | ||
return | ||
|
||
module_name = ".".join(modpath_from_file(module.file)) | ||
if level > module_name.count("."): | ||
return | ||
|
||
slice_index = 0 | ||
current_level = 0 | ||
for char in reversed(module_name): | ||
if current_level >= level: | ||
break | ||
if char == ".": | ||
current_level += 1 | ||
|
||
slice_index += 1 | ||
|
||
root_module = module_name[:-slice_index] if slice_index else module_name | ||
|
||
modname_separator = "" | ||
modname = getattr(node, "modname", "") | ||
if getattr(node, "modname", False): | ||
self.imports.add(f"{root_module}.{modname}") | ||
modname_separator = "." | ||
|
||
for name in node.names: | ||
self.imports.add(f"{root_module}{modname_separator}{modname}.{name[0]}") | ||
|
||
@only_required_for_messages("unused-module") | ||
def visit_module(self, node): | ||
if EXCLUDED.search(node.file): | ||
return | ||
|
||
self.modules.add(node) | ||
|
||
@only_required_for_messages("unused-module") | ||
def visit_importfrom(self, node): | ||
self.store_imported_modules(node) | ||
|
||
@only_required_for_messages("unused-module") | ||
def visit_import(self, node): | ||
self.store_imported_modules(node) | ||
|
||
@only_required_for_messages("unused-module") | ||
def close(self): | ||
for module in self.modules: | ||
if module.name not in self.imports: | ||
self.add_message("unused-module", node=module) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import models |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
'name': 'Test Import Messages', | ||
'license': 'AGPL-3', | ||
'author': 'Vauxoo, Odoo Community Association (OCA)', | ||
'version': '11.0.0.0.0', | ||
'depends': [ | ||
'base', | ||
], | ||
'data': [], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
class MyController(odoo.http.Controller): | ||
@odoo.http.route('/some_url', auth='public') | ||
def handler(self): | ||
return True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
print("Hello") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import res_partner |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from odoo.models import BaseModel | ||
|
||
class FailModel(BaseModel): | ||
_name = "fail.model" | ||
|
||
not_imported = fields.Boolean(default=True) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
THREE = 2 + 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
from odoo import fields | ||
from odoo.models import BaseModel | ||
|
||
from ..useful import USEFUL | ||
import foo | ||
|
||
|
||
class ResPartner(BaseModel): | ||
_name = "res.partner" | ||
|
||
random_field = fields.Char(string=USEFUL, size=foo.THREE) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
RANDOM_STRING = "HELLO" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import test_res_partner |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from ..models import unused_utils | ||
|
||
|
||
class TestResPartner: | ||
|
||
def test_something(self): | ||
return unused_utils.RANDOM_STRING |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
USEFUL = "foo" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import pass_wizard |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from odoo import fields, models | ||
|
||
|
||
class FailWizard(models.AbstractModel): | ||
_name = "fail.wizard" | ||
|
||
name = fields.Char() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
from odoo.models import AbstractModel | ||
from odoo import fields | ||
|
||
from .utils import func | ||
|
||
|
||
class PassWizard(AbstractModel): | ||
_name = "pass.wizard" | ||
|
||
name = fields.Char(required=True) | ||
|
||
def foo(self): | ||
return func(self) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
def func(*_args): | ||
return False |
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.
Why a new file? Shouldn't you implement new lints into the existing file for Odoo addons?
@moylop260 I don't have enough context to know if this is correct.
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.
I think odoo_addons is getting too big and grouping lints inside different checkers could help organization.