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

[ADD] New unused-module check #449

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ translation-required | String parameter on "%s" requires translation. Use %s_(%s
translation-too-few-args | Not enough arguments for odoo._ format string | E8306
translation-too-many-args | Too many arguments for odoo._ format string | E8305
translation-unsupported-format | Unsupported odoo._ format character %r (%#02x) at index %d | E8300
unused-module | This python module is not imported and has no effect | E8401
use-vim-comment | Use of vim comment | W8202
website-manifest-key-not-valid-uri | Website "%s" in manifest key is not a valid URI | W8114

Expand Down Expand Up @@ -257,6 +258,7 @@ Checks valid only for odoo <= 13.0
* missing-readme

- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/broken_module/__openerp__.py#L2 Missing ./README.rst file. Template here: https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst
- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/unused_module/__manifest__.py#L1 Missing ./README.rst file. Template here: https://github.com/OCA/maintainer-tools/blob/master/template/module/README.rst

* missing-return

Expand Down Expand Up @@ -292,6 +294,7 @@ Checks valid only for odoo <= 13.0
* print-used

- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/test_module/except_pass.py#L20 Print used. Use `logger` instead.
- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/unused_module/migrations/11.0.1.0.1/pre-migration.py#L1 Print used. Use `logger` instead.

* renamed-field-parameter

Expand Down Expand Up @@ -371,6 +374,12 @@ Checks valid only for odoo <= 13.0

- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/broken_module/models/broken_model.py#L478 Unsupported odoo._ format character 'y' (0x79) at index 30

* unused-module

- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/broken_module/coding_latin.py#L1 This python module is not imported and has no effect
- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/broken_module/encoding_utf8.py#L1 This python module is not imported and has no effect
- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/broken_module/interpreter_wox.py#L1 This python module is not imported and has no effect

* use-vim-comment

- https://github.com/OCA/pylint-odoo/blob/v9.0.5/testing/resources/test_repo/broken_module/pylint_oca_broken.py#L108 Use of vim comment
Expand Down
1 change: 1 addition & 0 deletions src/pylint_odoo/checkers/__init__.py
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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

125 changes: 125 additions & 0 deletions src/pylint_odoo/checkers/import_checker.py
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",
Copy link
Contributor

Choose a reason for hiding this comment

The 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. odoo-model-not-imported.

What do you think?

Copy link
Contributor Author

@antonag32 antonag32 Jun 13, 2023

Choose a reason for hiding this comment

The 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 .py file.

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)
2 changes: 2 additions & 0 deletions src/pylint_odoo/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ def register(linter):
linter.register_checker(checkers.odoo_addons.OdooAddons(linter))
linter.register_checker(checkers.vim_comment.VimComment(linter))
linter.register_checker(checkers.custom_logging.CustomLoggingChecker(linter))
linter.register_checker(checkers.import_checker.ImportChecker(linter))

# register any checking fiddlers
apply_augmentations(linter)
Expand All @@ -18,6 +19,7 @@ def get_all_messages():
all_msgs.update(checkers.odoo_addons.ODOO_MSGS)
all_msgs.update(checkers.vim_comment.ODOO_MSGS)
all_msgs.update(checkers.custom_logging.ODOO_MSGS)
all_msgs.update(checkers.import_checker.ODOO_MSGS)
return all_msgs


Expand Down
1 change: 1 addition & 0 deletions testing/resources/test_repo/unused_module/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from . import models
10 changes: 10 additions & 0 deletions testing/resources/test_repo/unused_module/__manifest__.py
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': [],
}
Empty file.
4 changes: 4 additions & 0 deletions testing/resources/test_repo/unused_module/controllers/test.py
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)
1 change: 1 addition & 0 deletions testing/resources/test_repo/unused_module/models/foo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
THREE = 2 + 1
11 changes: 11 additions & 0 deletions testing/resources/test_repo/unused_module/models/res_partner.py
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
1 change: 1 addition & 0 deletions testing/resources/test_repo/unused_module/useful.py
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()
13 changes: 13 additions & 0 deletions testing/resources/test_repo/unused_module/wizard/pass_wizard.py
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)
2 changes: 2 additions & 0 deletions testing/resources/test_repo/unused_module/wizard/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func(*_args):
return False
Loading
Loading