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

module_analysis does not count code lines from odoo core when installed from the debian package #3042

Open
fcayre opened this issue Oct 3, 2024 · 3 comments
Labels

Comments

@fcayre
Copy link
Member

fcayre commented Oct 3, 2024

Module

module_analysis

Describe the bug

To Reproduce

Affected versions: all

Steps to reproduce the behavior:

  1. Prerequisites: Odoo is installed using the debian package; module_analysis is installed
  2. Go to the account_cancel module page (in Applications app) in debug mode
  3. Press the "Refresh Code Analysis" button in the "Technical data" tab

Expected behavior
The Python Code Quantity field's value is 0 also it should not, as there are non-empty python files in the module (models/account_bank_statement.py).

Additional context
Debugging shows that this line is a true condition when odoo is installed by a debian package (root = /usr/lib/python3/dist-packages/odoo/addons/account_cancel), because exclude_directories value is ['lib', 'demo', 'test', 'tests', 'doc', 'description'], so the test returns {'lib'}

The exclude_directories comes from ir.config_parameter module_analysis.exclude_directories which interpretation is questionable. As a user of the module I expect its value to list folders relative to the module's folder, while in the code if any of the listed folder is found in the module's root path the entire module will be ignored.

I'm working on a PR to fix this.

@fcayre fcayre added the bug label Oct 3, 2024
@fcayre
Copy link
Member Author

fcayre commented Oct 3, 2024

@legalsylvain any thoughts on the module_analysis.exclude_directories parameter interpretation? Am I right and can I go on with the PR?

@legalsylvain
Copy link
Contributor

Hi @fcayre. Thanks for the investigation, regarding your problem.
Well, why that default values ? It is about what you want to measure.

In my thought, I wanted to measure the "amount of algorithmic complexity to be maintained".

For that reasons :

  • I proposed to ignore test / tests files that sometimes add a lot of code, and because if a module has a lot of tests, it is not less maintainable than a module that doesn't have any test. (in my opinion, it's even a bit the opposite)

  • Similar reasoning for doc / description and demo folders. It improves quality of modules. (better understanding of the module, easy to test for functional people, etc...)

  • in Odoo, "lib" folder contains extra libraries that comes from other projects. typical exemple, extra js lib used by "web" modules. https://github.com/odoo/odoo/tree/16.0/addons/web/static/lib. Just because the web module depends on jquery doesn't mean it adds complexity (the technical debt is carried by the jquery development team's maintenance team).
    Then because it wouldn't be very ‘fair’: adding a dependency to a python library adds a line to a requirements.txt file, so adding a javascript dependency should have the same behaviour. (no impact on the measure)

you can see the original PR, with some thought about what should contains or not the "excude_directories" values : #1618 (comment)

my opinion :
people want to measure differents things, and all values is legit. Finally, it is just a default value.
So, I propose to not change the default values, but add a text in the configure.rst file, to explain how to configure the module in case you install odoo core via debian, with a link to thisi current issue.
If agree could you make the PR in the last recent version of the module, so the text will be conserved when migrating the module.
https://github.com/OCA/server-tools/tree/17.0/module_analysis

fcayre added a commit to fcayre/server-tools that referenced this issue Oct 3, 2024
@fcayre
Copy link
Member Author

fcayre commented Oct 3, 2024

@legalsylvain I don't want to change the default value (which looks good to me) but the code, which does not interpret that value as it should in my opinion. See my PR for a better understanding of my objection.

fcayre added a commit to fcayre/server-tools that referenced this issue Oct 3, 2024
fcayre added a commit to fcayre/server-tools that referenced this issue Oct 3, 2024
fcayre added a commit to fcayre/server-tools that referenced this issue Oct 3, 2024
fcayre added a commit to fcayre/server-tools that referenced this issue Oct 3, 2024
legalsylvain pushed a commit that referenced this issue Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants