[ADD] hooks: New unused python file check #61

antonag32 · 2022-11-11T22:13:33Z

Unused python files are considered python files not imported in their corresponding init.py file. This only applies to actual files that need to be imported, as it only checks python files in standard locations that must be imported by odoo. This includes the following folders:

controllers
models
report
wizard
tests (only if it starts with test_)

Related to: Vauxoo/pylint-odoo#177

I decided against placing this checker in pylint since the entry point for it would be ugly and probably involve non standard code (aka can't be easily done with visit_ functions). I also did not introduce this into an existing hook since its correct functioning depends on the right files setting on pre-commit-hooks.yaml.

The code structure does not fill very well with existing code, but I think it might fit well after a restructuring, and I personally believe this is the most correct way to implement the desired check.

When run against Odoo 16.0 I get the following output:

Check unused python files................................................Failed
- hook id: oca-check-unused-python-file
- exit code: 255

addons/microsoft_calendar/tests/test_sync_odoo2microsoft.py: not imported
addons/microsoft_calendar/tests/test_sync_microsoft2odoo.py: not imported
odoo/addons/base/tests/test_uninstall.py: not imported
addons/lunch/tests/test_ui.py: not imported
odoo/tests/test_module_operations.py: not imported
odoo/addons/base/tests/test_mail_examples.py: not imported
addons/l10n_it_edi/tests/test_res_partner.py: not imported

I don't know if Odoo did that on purpose but if they were planning on those tests being run, they won't, at least according to their documentation.

Unused python files are considered python files not imported in their corresponding __init__.py file. This only applies to actual files that need to be imported, and automatically excludes folders like migration or __manifest__.py files

moylop260 · 2022-11-12T00:26:32Z

I think the best place to create python lints is pylint

It could be done using def open or using def visit_module in order to get all imported nodes for a particular module directory

Could we discuss it, please?

moylop260 · 2022-11-12T01:29:34Z

.pre-commit-hooks.yaml

+  entry: oca-check-unused-python-file
+  language: python
+  types: [python]
+  files: (controllers|models|report|wizard)\.py$|tests\/test_.*\.py$


We have hooks.py not considered here

There are cases where the developer use plural or singular names or even use the root path of the module to create files directly

Only be aware to discard known cases:

__init__.py

migrations/

True, I missed hooks.py.

Initially I went with the exclude route, only excluding __init__.py, migrations/ and __manifest__.py, but if you run the hook against Odoo, you will find there are tons of false positives, for example the drivers for IoT are python files and I am not sure they need to be imported, and they are also in non standard paths.

https://github.com/odoo/odoo/blob/16.0/addons/hw_drivers/iot_handlers/drivers/KeyboardUSBDriver.py

That is just an example, so I decided to instead just focus on well known folders that we know you must include all files from, which feels cleaner than excluding a few well known folders that we know you don't have to include files from.

src/oca_pre_commit_hooks/check_unused_python_file.py

moylop260 · 2022-11-12T01:39:24Z

tests/test_check_unused_python_file.py

+    return [f"{os.path.join(basedir, filename)}.py" for filename in filenames]
+
+
+def test_all_used_files(tmpdir):


Why are you not using unittest?

Could you use it?

Use a setUp to create tempdir and use a tearDown to delete it

Similar to https://github.com/Vauxoo/pre-commit-vauxoo/blob/332a706097926863d0235630fc3731a27ad4f559/tests/test_pre_commit_vauxoo.py#L21

and https://github.com/Vauxoo/pre-commit-vauxoo/blob/332a706097926863d0235630fc3731a27ad4f559/tests/test_pre_commit_vauxoo.py#L40-L41

BTW I didn't find where tmpdir variable is assigned

Since we are using pytest I did not find it necessary to use the old unittest way. Using a setUp and tearDown is not necessary since pytest already does this for us, that is why I am using tmpdir which automatically creates a temporary directory, that option is deprecated tho so I need to change it to tmp_path.

https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html

Also, I find it cleaner to separate test files per functionality being tested instead of placing all tests in the same file so I placed it separately in order to raise a point about it (although I guess this is just personal preference).

I was also thinking that perhaps our main way of testing as of right now (running and comparing outputs against a test files) is more of an integration test rather than a unit test, and perhaps we could keep our current tests (but treat them as integration tests), and write more modular unit tests like the one in this PR for each block/check.

Just an idea I think we could consider for the reestructuring

moylop260 · 2022-11-12T01:41:39Z

tests/test_check_unused_python_file.py

+    assert main(filepaths) == -1
+
+
+def test_complex_init(tmpdir):


Could you add a unittest to silent valid cases?

I mean, imagine you have a odoo module to process a python file with a dict instead of a json (similar way __manifest__.py) so you will have python files without importing but not default one

I think the best place to create python lints is pylint

It could be done using def open or using def visit_module in order to get all imported nodes for a particular module directory

Could we discuss it, please?

Yes, ideally Python code should be in pylint, I agree. I went with hooks since this PR was a joy to code haha, I feel the resulting code is clean and elegant, and the files and exclude options are helpful. I will look into moving it with the help of visit_module, I did not think of that before and it seems promising.

moylop260 · 2022-11-12T01:50:55Z

src/oca_pre_commit_hooks/check_unused_python_file.py

+
+def main(argv: Union[List[str], None] = None) -> int:
+    parser = argparse.ArgumentParser()
+    parser.add_argument("filenames", nargs="*")


Could you consider to add an extra parameter to change the exit_status in order to be able to enable it but without fails, please?

Similar to:
https://github.com/PyCQA/bandit/blob/d9fe642e01866e460454641bcd14b9de9d2b1478/bandit/cli/main.py#L363-L369

I think the behavior for all these hooks should be standard. I understand we are the main users for it and changes like this can blow up our whole CI, but if we separate concerns, I think that is not a concern of this repo, but instead of the users (us).

There is already a "native" option for users to run hooks but make them optional, I think we could use that instead of changing the code.

https://stackoverflow.com/questions/59730765/is-it-possible-to-run-mypy-pre-commit-without-making-it-fail

moylop260 · 2022-11-12T01:54:15Z

src/oca_pre_commit_hooks/check_unused_python_file.py

+def get_imported_files(dirname: str) -> Set[str]:
+    imported_files = set()
+    init_file = os.path.join(dirname, "__init__.py")
+    if os.path.exists(init_file):


Prefer return early

Suggested change

if os.path.exists(init_file):

if not os.path.exists(init_file):

return imported_files

moylop260 · 2022-11-12T01:56:34Z

Do you think it could be good to create a new PR to https://github.com/pre-commit/pre-commit-hooks?

But using a generic way

antonag32 · 2022-11-12T19:44:44Z

Do you think it could be good to create a new PR to https://github.com/pre-commit/pre-commit-hooks?

But using a generic way

Maybe but I think importing stuff from __init__.py only applies to Odoo projects, at least based on my experience working with non-odoo Python projects, so perhaps it is not generic enough for that repo. Unless I am missing a generic way that this can be useful to most people working in non-odoo projects too.

antonag32 · 2022-11-23T02:25:26Z

Replaced by: OCA/pylint-odoo#449

Pylint is a much better option, since imports and classdefs can be checked which means the check no longer depends on customs being followed.

antonag32 force-pushed the unused-python-file-anton branch 3 times, most recently from ac1b381 to faeb201 Compare November 11, 2022 22:29

[ADD] hooks: New unused python file check

5bf9b76

Unused python files are considered python files not imported in their corresponding __init__.py file. This only applies to actual files that need to be imported, and automatically excludes folders like migration or __manifest__.py files

antonag32 force-pushed the unused-python-file-anton branch from faeb201 to 5bf9b76 Compare November 11, 2022 22:35

moylop260 reviewed Nov 12, 2022

View reviewed changes

antonag32 closed this Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADD] hooks: New unused python file check #61

[ADD] hooks: New unused python file check #61

antonag32 commented Nov 11, 2022

moylop260 commented Nov 12, 2022

moylop260 Nov 12, 2022

antonag32 Nov 12, 2022

moylop260 Nov 12, 2022

moylop260 Nov 12, 2022

moylop260 Nov 12, 2022

antonag32 Nov 12, 2022

antonag32 Nov 12, 2022

moylop260 Nov 12, 2022

antonag32 Nov 12, 2022

moylop260 Nov 12, 2022

antonag32 Nov 12, 2022

moylop260 Nov 12, 2022

moylop260 commented Nov 12, 2022 •

edited

Loading

antonag32 commented Nov 12, 2022

antonag32 commented Nov 23, 2022 •

edited

Loading

		return [f"{os.path.join(basedir, filename)}.py" for filename in filenames]


		def test_all_used_files(tmpdir):

	if os.path.exists(init_file):
	if not os.path.exists(init_file):
	return imported_files

[ADD] hooks: New unused python file check #61

[ADD] hooks: New unused python file check #61

Conversation

antonag32 commented Nov 11, 2022

moylop260 commented Nov 12, 2022

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

moylop260 commented Nov 12, 2022 • edited Loading

antonag32 commented Nov 12, 2022

antonag32 commented Nov 23, 2022 • edited Loading

moylop260 commented Nov 12, 2022 •

edited

Loading

antonag32 commented Nov 23, 2022 •

edited

Loading