[ADD] New unused-module check #449

antonag32 · 2022-11-23T02:23:30Z

This new check finds all files that have a class which inherits from one of Odoo's models classes and ensures they are imported.

This is required since classes that interact with the ORM (create models, or inherit them) MUST be imported in the corresponding init.py file, otherwise they won't have an effect.

antonag32 · 2022-11-23T02:24:39Z

Fixes part of Vauxoo#177. Empty files should be checked with a pre-commit hook instead.

antonag32 · 2022-12-13T17:18:58Z

@moylop260 Could you review please

github-actions · 2023-04-16T12:13:29Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

antonag32 · 2023-05-24T01:49:02Z

Should this be revived? @moylop260 @luisg123v

moylop260 · 2023-05-30T23:41:11Z

@luisg123v

Please check

luisg123v

Please add comments to the code, code is not trivial and there's not a single comment

luisg123v · 2023-05-31T00:01:05Z

testing/resources/test_repo/imports_module/__manifest__.py

@@ -0,0 +1,10 @@
+{
+    'name': 'Test Import Messages',


Are you sure you need a new module?

CAn't you reuse some of the already existing broken modules?

I think it is cleaner to have test files for each message, or at least have them groupd. pylint does some variation of this and I find it easier to work with, since you don't have to look around different broken modules to find the offending code.

I think differently, creating a new module will trigger errors in other checkers. Actually, you had to modify unrelated expected errors for this to work.

True, as long as the test sources are shared between checkers adding new checkers/modifying them runs the risk of breaking the whole thing. This is unrelated to the issue itself so I guess I will just use the errors that naturally pop up in existing broken modules instead of creating new files (brings no real improvement as of now)

luisg123v · 2023-05-31T00:06:59Z

src/pylint_odoo/checkers/__init__.py

@@ -1,3 +1,4 @@
 from . import odoo_addons
 from . import vim_comment
 from . import custom_logging
+from . import import_checker


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.

I think odoo_addons is getting too big and grouping lints inside different checkers could help organization.

moylop260 · 2022-11-23T03:42:55Z

src/pylint_odoo/checkers/import_checker.py

+            if import_stmt.modname == "odoo":
+                if any("models" in name for name in import_stmt.names):
+                    results.add("models")
+            elif import_stmt.modname == "odoo.models":


Is it not the same than?

pylint-odoo/src/pylint_odoo/checkers/odoo_addons.py

Lines 1440 to 1442 in dd98797

if "odoo" in package_names:

class_base_name = class_base.as_string().split(".")[-1]

if class_base_name in ["Model", "AbstractModel", "TransientModel"]:

moylop260 · 2022-11-23T03:44:31Z

src/pylint_odoo/checkers/import_checker.py

+    @lru_cache()
+    def _get_odoo_import_orm_names(self, module: Module) -> Set[str]:
+        results = set()
+        for import_stmt in filter(lambda node: isinstance(node, ImportFrom), module.body):


I think you are redundant the visit here

I mean, you have a classdef then get the parent then filter all the module.body the ImportForm nodes

But then you have visit_importfrom
It should contains the same than this one

Is not?

True, I think I can skip this and just use visit_importfrom, will look into it

moylop260 · 2022-11-23T03:48:34Z

src/pylint_odoo/checkers/import_checker.py

+        odoo_imports = self._get_odoo_import_orm_names(node.parent)
+        if "models" in odoo_imports:
+            not_orm = not any(
+                (basename.startswith("models.") or basename in odoo_imports) for basename in node.basenames


Think this lint could be good in the pycqa/pylint project

Could you remove the models things related, please?

In fact, The original issue is not related with models but controllers

I think this check should be more standard than only models

I mean, if the folder has a __init__.py so get all the .py files in the same path vs the visit_importfrom

Without visiting a particular classdef

Maybe pycqa/pylint could adopt it

The known valid cases are:
migrations/ where doesn't have a __init__.py file
__init__.py must be auto-discarded
__manifest__.py / __openerp__.py use an extra option to discard names

Sorry, what would be the generic check exactly? AFAIK outside of Odoo it is normal to have empty init.py files, so I don't think it would be helpful.

Also I did the models thing to extend it and make sure that both controllers and models are imported in their __init__.py so they make an effect.

If I add a utility class inside the models folder, it is not required to import it in the __init__.py, I think that is why I checked for class defs.

I think I understand now, do you mean checking that all .py files are imported somewhere (referenced)? In the case of Odoo I think some need to be imported in __init__.py to make an effect but I can now see how it may be generic enough.

I will look at existing pylint messages to see if I can find something similar.

moylop260 · 2023-05-31T03:58:56Z

testing/resources/test_repo/imports_module/models/fail_model.py

@@ -0,0 +1,6 @@
+from odoo.models import BaseModel


Could you test if the disable-check-comment works, please?

antonag32 · 2023-06-01T03:35:00Z

@moylop260 I have refactored the code to make it more generic and applicable to all files (except those that we agreed to exclude). I also opened an issue on pylint so lets see where that goes.

The pipeline will be all red because I had some trouble running tox, I plan to fix that and tidy up the MR tomorrow.

antonag32 · 2023-06-02T01:17:49Z

I had to make a small refactoring to prevent this and future messages from destroying the whole test suite (the new check was triggering on existing broken modules and the new source code was triggering other checkers). As a bonus point messages that don't need a whole module can now use single files (like pylint does).

As for the checker, it is mostly complete, just need to take into account import statements as well to have every possible case covered.

antonag32 · 2023-06-05T18:24:16Z

@moylop260 @luisg123v

I think everything is ready.

Works generally (not only for ORM models)
Added comments to document inner workings
Support for both from x import y and import x statements
Test module to verify all possible cases:
- tests are excluded
- migrations as well
- works for all modules (controllers, models, and everything else)
- takes into account relative import paths
I tested it locally on a production repo from a client and no false positives were raised. Messages were correctly raised when I introduced errors of this kind into the repo as well.

moylop260 · 2023-06-12T19:37:51Z

@luisg123v

Could you review it, please?

luisg123v · 2023-06-12T21:19:54Z

src/pylint_odoo/checkers/__init__.py

-from . import odoo_addons
-from . import vim_comment
-from . import custom_logging
+from . import custom_logging, import_checker, odoo_addons, vim_comment


Why now in one line? I think this is not expected

luisg123v · 2023-06-12T21:23:09Z

src/pylint_odoo/checkers/import_checker.py

+    # C->convention R->refactor W->warning E->error F->fatal
+    "E8401": (
+        "This python module is not imported and has no effect",
+        "unused-module",


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?

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

luisg123v · 2023-06-12T21:25:58Z

src/pylint_odoo/checkers/import_checker.py

+
+    @staticmethod
+    def get_module_from_node(node, max_attempts: int = 15) -> Union[None, Module]:
+        """Get the module a node belongs to.


Fix semantic, this is not clear

Did you mean "Return whether the module "a" belongs to the provided node"?

I'm not sure

luisg123v · 2023-06-12T21:26:36Z

src/pylint_odoo/checkers/import_checker.py

+        """Get the module a node belongs to.
+
+        :param node: Node that belongs to the module to be extracted.
+        :param int max_attempts: Amount of attempts that will be made to obtain the module. Nodes that are


amount -> number

luisg123v · 2023-06-12T21:26:47Z

src/pylint_odoo/checkers/import_checker.py

+
+        :param node: Node that belongs to the module to be extracted.
+        :param int max_attempts: Amount of attempts that will be made to obtain the module. Nodes that are
+        nested deeper than max_attempts won't be found.


luisg123v · 2023-06-12T21:30:22Z

src/pylint_odoo/checkers/import_checker.py

+        module = ImportChecker.get_module_from_node(node)
+        if not module:
+            return
+        if "tests" in module.name:


Join with the above conditional

luisg123v · 2023-06-12T21:32:28Z

src/pylint_odoo/checkers/import_checker.py

+            return
+
+        if isinstance(node, ImportFrom):
+            level = 0 if not node.level else node.level


prefer positive conditions instead of negative ones

It's more readable:

if condition: x = 1 else: x = 2

than:

if not condition: x = 2 else: x = 1

Because else would be a double negation

luisg123v · 2023-06-12T21:33:28Z

src/pylint_odoo/checkers/import_checker.py

+        elif isinstance(node, Import):
+            level = 1
+        else:
+            raise ValueError("Node must be either ImportFrom or Import")


I don't think you should raise exceptions inside lints.

luisg123v · 2023-06-12T21:34:21Z

src/pylint_odoo/checkers/import_checker.py

+
+            slice_index += 1
+
+        if slice_index != 0:


Suggested change

if slice_index != 0:

if slice_index:

luisg123v · 2023-06-12T21:36:12Z

src/pylint_odoo/checkers/import_checker.py

+        if slice_index != 0:
+            root_module = module_name[:-slice_index]
+        else:
+            root_module = module_name


Use inline conditional

luisg123v · 2023-06-12T21:54:12Z

another excluded directory: doc

antonag32 · 2023-06-13T17:39:40Z

@luisg123v I have made the requested changes, the only thing left is my question about the code on docstrings.

github-actions · 2024-02-11T12:12:58Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

luisg123v

This needs local rebase.

Previously faulty code which generated messages was distributed throught multiple repositories. Tests based themselves on the total amount of expected messages contained within the repositories. This is not sustainable because adding new checks oftentime triggers unintended messages on all repos, breaking tests and costing developer time. This commit freezes test repos to keep backwards compatibility, however all new checks must implement their own separate test sources, this will speed up development and help organization.

This new check looks for python files (modules) which are not referenced anywhere in the codebase, making them useless. Closes Vauxoo#177.

antonag32 · 2024-02-14T01:41:23Z

I have rebased it. I have also realized I have become a less destructive person since I am double thinking the frozen tests thing. I still think it makes sense to keep test files separately, since that reduces lots of headaches and means adding new messages does not blow up on old test files.

However I wonder if there is any cleaner way? Maybe it is better to just keep rolling the old way even if it feels "wrong", it seems like the whole test suite was built around the old way.

github-actions · 2024-06-23T12:15:57Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

antonag32 mentioned this pull request Nov 23, 2022

[ADD] hooks: New unused python file check OCA/odoo-pre-commit-hooks#61

Closed

antonag32 requested a review from moylop260 November 23, 2022 17:14

antonag32 force-pushed the 177-anton branch from 8ebb17c to d862e56 Compare November 23, 2022 20:08

moylop260 self-assigned this Dec 13, 2022

moylop260 added the needs review label Dec 13, 2022

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 16, 2023

github-actions bot closed this May 21, 2023

moylop260 reopened this May 30, 2023

luisg123v suggested changes May 31, 2023

View reviewed changes

moylop260 reviewed May 31, 2023

View reviewed changes

antonag32 changed the title ~~[ADD] New missing-python-import check~~ [ADD] New unused-module check Jun 1, 2023

antonag32 force-pushed the 177-anton branch from d862e56 to 910a8ee Compare June 1, 2023 03:32

antonag32 force-pushed the 177-anton branch 2 times, most recently from 5aa8f7c to 57f80d7 Compare June 2, 2023 01:05

antonag32 force-pushed the 177-anton branch 4 times, most recently from e33765b to ef601e1 Compare June 2, 2023 23:43

github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 4, 2023

luisg123v suggested changes Jun 12, 2023

View reviewed changes

antonag32 force-pushed the 177-anton branch 2 times, most recently from 2cac199 to 0747f2b Compare June 13, 2023 17:29

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 11, 2024

luisg123v suggested changes Feb 12, 2024

View reviewed changes

antonag32 added 2 commits February 13, 2024 19:15

[ADD] new unused-module check

a45128f

This new check looks for python files (modules) which are not referenced anywhere in the codebase, making them useless. Closes Vauxoo#177.

antonag32 force-pushed the 177-anton branch from 0747f2b to a45128f Compare February 14, 2024 01:22

github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 18, 2024

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 23, 2024

github-actions bot closed this Jul 23, 2024

	if "odoo" in package_names:
	class_base_name = class_base.as_string().split(".")[-1]
	if class_base_name in ["Model", "AbstractModel", "TransientModel"]:

[ADD] New unused-module check #449

[ADD] New unused-module check #449

Conversation

antonag32 commented Nov 23, 2022

antonag32 commented Nov 23, 2022

antonag32 commented Dec 13, 2022

github-actions bot commented Apr 16, 2023

antonag32 commented May 24, 2023

moylop260 commented May 30, 2023

luisg123v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonag32 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

antonag32 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

antonag32 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonag32 commented Jun 1, 2023

antonag32 commented Jun 2, 2023 • edited Loading

antonag32 commented Jun 5, 2023 • edited Loading

moylop260 commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonag32 Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisg123v commented Jun 12, 2023

antonag32 commented Jun 13, 2023

github-actions bot commented Feb 11, 2024

luisg123v left a comment

Choose a reason for hiding this comment

antonag32 commented Feb 14, 2024

github-actions bot commented Jun 23, 2024

antonag32 May 31, 2023 •

edited

Loading

antonag32 May 31, 2023 •

edited

Loading

antonag32 May 31, 2023 •

edited

Loading

antonag32 commented Jun 2, 2023 •

edited

Loading

antonag32 commented Jun 5, 2023 •

edited

Loading

antonag32 Jun 13, 2023 •

edited

Loading