OCA / pylint-odoo

Odoo plugin for Pylint
http://www.pylint.org
143 stars 168 forks source link

[IMP] add support to Odoo v17.0 #477

Closed celm1990 closed 11 months ago

celm1990 commented 11 months ago

closes https://github.com/OCA/pylint-odoo/issues/476

@antonag32 can you review please?

pedrobaeza commented 11 months ago

@sbidoul I think this means a release of the library should be done and we should change the pre-commit config of all of the 17.0 branches.

sbidoul commented 11 months ago

@pedrobaeza I don't understand. This is PR just changes the default value for the list of allowed versions, and on 17.0 branches we have the correct value: https://github.com/OCA/mis-builder/blob/cf03aa9a706b9cb678ab4fd57f901852e4df007e/.pylintrc#L13

pedrobaeza commented 11 months ago

Please check the origin issue. It seems the checks are not being done correctly on current pylint-odoo version for 17.0.

sbidoul commented 11 months ago

Yeah I know, but I doubt this PR would fix that.

pedrobaeza commented 11 months ago

Well, according one Vauxoo's guy in #478, it seems this is the way to go.

celm1990 commented 11 months ago

Yeah I know, but I doubt this PR would fix that.

On my repo for example, I changed pre-commit configuration and now this working

image

moylop260 commented 11 months ago

@antonag32

Could you check why this PR is fixing the error even if the pylint.cfg has the value explicitly assigned, please?

https://github.com/celm1990/repo_with_ruff/blob/c46854ef3685bbd413277f22e6fb0dc72382484a/.pylintrc-mandatory#L12

Even if we should support the new version by default, the explicitly value assigned should work too as @sbidoul said

antonag32 commented 11 months ago

@antonag32

Could you check why this PR is fixing the error even if the pylint.cfg has the value explicitly assigned, please?

https://github.com/celm1990/repo_with_ruff/blob/c46854ef3685bbd413277f22e6fb0dc72382484a/.pylintrc-mandatory#L12

Even if we should support the new version by default, the explicitly value assigned should work too as @sbidoul said

It is this piece of code: https://github.com/OCA/pylint-odoo/blob/9d85855b850d23d9004ac27b2e128b678888a19d/src/pylint_odoo/checkers/odoo_base_checker.py#L35-L40

The condition <= misc.version_parse(odoo_maxversion) is not met since odoo_maxversion is 16.0 as of now, and odoo_version is set as 17.0 on the config file.

Because is_odoo_message_enabled gets called when adding new messages: https://github.com/OCA/pylint-odoo/blob/9d85855b850d23d9004ac27b2e128b678888a19d/src/pylint_odoo/checkers/odoo_base_checker.py#L42-L46

No messages will ever get added.

antonag32 commented 11 months ago

I wen't with adding 17.0 because it was a simple fix which fits with the current program logic. We could look into making pylint-odoo future proof and not require this sort of changes (adding new odoo versions), but I don't know if the project is interested in that (cost-benefit).

I looked at is_odoo_message_enabled and could not think of a fix to it that was as simple as the one in this PR. Maybe someone else can help.

moylop260 commented 11 months ago

@antonag32

Ok, thank you

Let's to do the quick fix then

moylop260 commented 11 months ago

FYI it will be released to https://pypi.org/project/pylint-odoo/9.0.2 after the following build finishes https://github.com/OCA/pylint-odoo/actions/runs/6906205867/job/18790705636

pedrobaeza commented 11 months ago

So we need to update all 17.0 branches pylint-odoo version, isn't it?

moylop260 commented 11 months ago

So we need to update all 17.0 branches pylint-odoo version, isn't it?

Hi @pedrobaeza

Yes, please

pedrobaeza commented 11 months ago

@sbidoul you have the tools for such massive upgrade... I can do the PR to oca-addons-template though.

sbidoul commented 11 months ago

@sbidoul you have the tools for such massive upgrade... I can do the PR to oca-addons-template though.

Will do. No urgency though, right?

pedrobaeza commented 11 months ago

No critical checks indeed.

celm1990 commented 11 months ago

for this PR https://github.com/Tecnativa/doodba-copier-template/pull/425 is need upgrade https://github.com/OCA/oca-addons-repo-template/pull/235 to pass test

pedrobaeza commented 11 months ago

@celm1990 I don't see why we need the OCA addons template for Doodba. Just put the new pylint-odoo version in Doodba copier template.

celm1990 commented 11 months ago

@celm1990 I don't see why we need the OCA addons template for Doodba. Just put the new pylint-odoo version in Doodba copier template.

@pedrobaeza because this test change version module and expected raise error, but pylint-odoo not raise error. Or did I misunderstand?

pedrobaeza commented 11 months ago

But as said, just bump the pylint-odoo version to 9.0.4 in Doodba.