search

OCA / odoo-pre-commit-hooks

Linters of Odoo addons that complement pylint-odoo
GNU Affero General Public License v3.0
10 stars 12 forks source link

Checks not disabled when using `--enable` #77

Closed antonag32 closed 1 year ago

antonag32 commented 1 year ago

Fastest way to reproduce:

Run oca-checks-po --enable=nonexistent --fix on a repository with .po files that are not formatted correctly (would trigger on po-pretty-format).

Expected behavior: Only checks mentioned in --enable should run, there are no checks named like that (nonexistent), so nothing should happen.

Actual behavior: Files are automatically fixed, but no error messages are printed. This means the check (pretty-format-po) is running, but its messages are being filtered out. Probably all checks are being run.

The two functions related to this (enabling/disabling and filtering out messages) are:

Probably a good idea to:

  1. Move utils.getattr_checks into an instance method for BaseChecker so that enable, disable no longer need to be passed in as args (can be taken directly from self)
  2. Remove the need for filter_checks_enabled_disabled by only adding messages when they are enabled (check before adding for cases where the method/check is called regardless of the current enabled/disabled messages)
moylop260 commented 1 year ago

FYI weird cases are reproduced using the following command:

~/odoo/ircanada 14.0$ oca-checks-odoo-module --disable=xml-oe-structure-missing-id *|grep qweb

The output shows xml-dangerous-qweb-replace-low-priority errors

But using: ~/odoo/ircanada 14.0$ oca-checks-odoo-module * |grep qweb

The output doesn't show xml-dangerous-qweb-replace-low-priority errors even if they were not disabled

antonag32 commented 1 year ago

Good news, we are not crazy and our unittests do work (disable and enable one by one). This is caused by the existence of .oca_hooks.cfg in the repositories we are testing on. What is needed to solve this is to merge config sources instead of completely replacing them.

Currently if there is a CLI --enable or --disable, the same settings on .oca_hooks.cfg will be completely disregarded. We need to merge them instead.

antonag32 commented 1 year ago

After further analysis (as stated above) there are no real issues with checks not being disabled. Merging configurations won't be done for the moment, therefore closing.