[ADD] prohibited-method-override: Add a new check to identify methods marked as prohibited to override

[Pexers]

Pull Request Template

Description

• Check for prohibited methods being overridden.
• Enables the user to specify, for example, core Odoo methods that should remain unaltered and cannot be overridden under any circumstances.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

1. There's a new test named test_175_prohibited_method_override within test_main.py.
2. Run the command below to start the test (using Python v3.9).

$ tox -e py39  -- -k test_175_prohibited_method_override

3. Verify that the test passes with two methods prohibited from being overridden. ✅

tests/test_main.py::MainTest::test_175_prohibited_method_override ************* Module broken_module.tests.test_model
testing/resources/test_repo/broken_module/tests/test_model.py:15 Prohibited override of "some_base_method_1" method. - [prohibited-method-override]
testing/resources/test_repo/broken_module/tests/test_model.py:15 Prohibited override of "some_base_method_2" method. - [prohibited-method-override]

PASSED

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[antonag32]

Thanks for the PR. I briefly went over your implementation and I have some questions.

It seems like your current code looks for super() calls to attributes in the prohibited override methods variable. If that is the case,
it does not take into account the method where it is being called. For example, is this overwriting anything?

def my_method(self):
    super().some_other_method()

It seems to me that an overwrite would look more like this:

def my_method(self):
    super().my_method()

I also don't have merging rights but I think this feature is too user specific, so maybe it makes sense to develop it as a separate plugin.

[Pexers]

Hey @antonag32, first and foremost, thanks for the quick reply! 😄

I've made a small change to fix the raised issue concerning attribute & parent method names not being equal.

In regards to this check being too user specific, I understand that overwriting Odoo core methods is part of the development workflow. However, I would like to raise awareness among developers within my organization that there are some base methods considered to be sensitive, and extra care should be taken when modifying them.

Nevertheless, this check won't raise any errors, only warnings, and it will remain optional. 🙂

[sconetto]

hello @antonag32 @Pexers,

I believe this is a very valid addition, we had some quality issues of engineers overriding base Odoo methods without the proper care of managing super calls. I feel this would be an awesome thing to had in the pylint-odoo.

👍🏻 +1

[moylop260]

@yajo @antonag32 @luisg123v

Please, review

[yajo]

LGTM, although I can't imagine a use case for this 🤷🏼‍♂️

[antonag32]

I guess the feature may be useful to some. However the user base that would benefit from it is probably relatively small so making sure the computations are not performed unless required is probably a good idea. As long as we follow that and keep the logic for this check simple (to avoid technical debt) I don't think it would be damaging to introduce this feature.

[luisg123v]

Could you squash commits and provide a meaningful commit description, please?

[luisg123v]

@Pexers the following was not addressed:

Could you squash commits and provide a meaningful commit description, please?

I think there should be only one commit.

[luisg123v]

We're almost done!

Just a couple of suggestions regarding commit message:

• It includes PR number, but it shouldn't
• Title is too long (103 characters). Commit titles are usually stripped to 72 characters. I'd recommend summarizing, e.g.:

[ADD] prohibited-method-override: new check to avoid overriding methods

[Pexers]

All done ✅ Thank you for your time guys! 😄

[luisg123v]

@Pexers thanks for contributing!

LGTM 👍

@moylop260 Could you merge, please?