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

pylint:disable deprecated. New oca-disable not working? #233

Closed
LoisRForgeFlow opened this issue Nov 13, 2023 · 8 comments
Closed

pylint:disable deprecated. New oca-disable not working? #233

LoisRForgeFlow opened this issue Nov 13, 2023 · 8 comments
Labels
bug Something isn't working stale PR/Issue without recent activity, it'll be soon closed automatically.

Comments

@LoisRForgeFlow
Copy link

LoisRForgeFlow commented Nov 13, 2023

Thanks for the continuous improvement year after year!

One question though. I am updating the ddmrp repository (branch 14.0 - OCA/ddmrp#332) and when running pre-commit I found the following deprecation warning:

ddmrp/ddmrp_purchase_hide_onhand_status/views/purchase_order_view.xml:2 WARNING. DEPRECATED. Use oca-disable instead.

However, as you can see in the PR, doing that replacement does not seem to work as now the xml-view-dangerous-replace-low-priority raises anyway: https://github.com/OCA/ddmrp/actions/runs/6851952066/job/18629369432?pr=332#step:7:101

As you can see in the specific file, there is a reason to disable it: OCA/ddmrp@00f030c#diff-b72aa136a90dd553777d52517e3063e85a751b083d38f277441559cec152ed1cR5

Anything else, I should do in order to disable that lint only in this file?

Thanks for your help.

@LoisRForgeFlow LoisRForgeFlow added the bug Something isn't working label Nov 13, 2023
@LoisRForgeFlow
Copy link
Author

Hi @sbidoul do you have any quick pointer in this one?

@sbidoul
Copy link
Member

sbidoul commented Nov 15, 2023

Hi Lois, I've not had time to look into this yet. Maybe the docs of https://github.com/OCA/odoo-pre-commit-hooks can help.

LoisRForgeFlow added a commit to ForgeFlow/ddmrp-1 that referenced this issue Nov 15, 2023
`pylint:disablep` throws a deprecated warning, however the new oca-disable
does not work as expected. The workaround is to disable the check for
oca-checks-odoo.

The same check is done by pytlint_odoo and in that case the old `pylint:disable`
syntax do have an effect.

See OCA/oca-addons-repo-template#233 for details.
@LoisRForgeFlow
Copy link
Author

LoisRForgeFlow commented Nov 15, 2023

@sbidoul Thanks for the pointer!

I did not realize that there are two hooks doing similar jobs. What I found:

I think this solution will allow me to move forward, however there is still the issue oca-hooks:disable syntax.

LoisRForgeFlow added a commit to ForgeFlow/ddmrp-1 that referenced this issue Nov 15, 2023
`pylint:disable` throws a deprecated warning, however the new oca-disable
does not work as expected. The workaround is to disable the check for
oca-checks-odoo.

The same check is done by pytlint_odoo and in that case the old `pylint:disable`
syntax do have an effect.

See OCA/oca-addons-repo-template#233 for details.
@sbidoul
Copy link
Member

sbidoul commented Nov 15, 2023

We should not have the same check in both hooks. It looks like our pylint-odoo + odoo-pre-commit-hooks configs need an in-depth review, as I'm afraid we could not keep up with the recent changes. @moylop260 can you help with that?

@antonag32
Copy link

Not moylop260, but I can help.

odoo-pre-commit-hooks was created to perform all the non-python checks pylint-odoo was performing (like XML checks). Once odoo-pre-commit-hooks was released, all non-python checks from pylint-odoo were removed.

This means that you are using an old version of pylint-odoo that was released before odoo-pre-commit-hooks (7.0.2). I cloned the ddmrp repository and after updating the hook's version to v9.0.1 there are no more XML checks by Ptython and no more duplicate messages.

As for oca-hooks:disable not working, indeed it seems like that was a bug. Based on my analysis it was fixed with this commit: OCA/odoo-pre-commit-hooks@0314fdf

So using the latest version should fix it (v0.0.29). The latest version also adds a new check: po-pretty-format, you can disable it using args (as I did) or through an .oca-hooks.cfg on your repository's root.

Overall, the errors you report were fixed with this diff (at least on my end):

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index d72a7bc..d742386 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -52,13 +52,11 @@ repos:
           - --if-source-changed
           - --keep-source-digest
   - repo: https://github.com/OCA/odoo-pre-commit-hooks
-    rev: v0.0.25
+    rev: v0.0.29
     hooks:
       - id: oca-checks-odoo-module
-        # While https://github.com/OCA/oca-addons-repo-template/issues/233
-        # is not fixed.
-        args: ["--disable=xml-view-dangerous-replace-low-priority"]
       - id: oca-checks-po
+        args: ["--disable=po-pretty-format"]
   - repo: https://github.com/myint/autoflake
     rev: v1.4
     hooks:
@@ -145,7 +143,7 @@ repos:
         name: flake8
         additional_dependencies: ["flake8-bugbear==20.1.4"]
   - repo: https://github.com/OCA/pylint-odoo
-    rev: 7.0.2
+    rev: v9.0.1
     hooks:
       - id: pylint_odoo
         name: pylint with optional checks

Try it and let me know how it goes.

@moylop260
Copy link
Contributor

moylop260 commented Nov 18, 2023

We should not have the same checks in both hooks. Our pylint-odoo + odoo-pre-commit-hooks configs need an in-depth review, as I'm afraid we could not keep up with the recent changes. @moylop260 can you help with that?

Check the following description OCA/pylint-odoo#396

All the pylint-odoo checks not related to python were removed as @antonag32 commented

They were migrated to oca-hooks (e.g. XML, PO...)

It means, that pylint-odoo < 8 has the same checks XML, PO checks than oca-hooks together

That is the reason if you use pylint-odoo < 8 + oca-hooks it will get wrong results because they together are not compatibles

You need to use pylint-odoo >= 8 if you want to use oca-hooks

Notice the following comment related with green results only using the correct versions:

@LoisRForgeFlow
Copy link
Author

Hi @antonag32 and @moylop260

Sorry, I for the late reply I typically miss messages when not directly pinged.

I tested your diff and it works, the questions is now: should we update those versions in the template? cc @sbidoul

If you think that is correct, I can do the PR.

Thanks!

Copy link

There hasn't been any activity on this issue in the past 6 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 issue to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 23, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

No branches or pull requests

4 participants