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

[FIX] pre-commit: update pylint odoo to enable valid-odoo-versions key for v18 #280

Conversation

sanderlienaerts
Copy link
Contributor

@sanderlienaerts sanderlienaerts commented Oct 11, 2024

Without updating the linter and using valid-odoo-versions=18.0, pylint_odoo (mandatory check) passes green but without checking any rules.

pylint-odoo unfortunately doesn't notify the user of 18.0 not being supported in v9.1.2. Support was added in v9.1.3 (OCA/pylint-odoo@v9.1.2...v9.1.3) in PR OCA/pylint-odoo#497

Example with valid-odoo-versions=18.0 and pylint-odoo==v1.9.2:
Screenshot 2024-10-11 at 13 54 37

Working example with valid-odoo-versions=18.0 and pylint-odoo==v1.9.3:
Screenshot 2024-10-11 at 13 53 57

Builds on: #272

pylint_odoo's version was also being overridden. This commit fixes that by adding a new v17 section.

@sanderlienaerts sanderlienaerts force-pushed the cf-pre-commit-update-pylint-odoo-to-enable-support-for-v18 branch from 2d884e3 to a32f783 Compare October 11, 2024 12:29
{%- set repo_rev.pyupgrade = "v2.38.2" %}
{%- set repo_rev.ruff = "v0.6.8" %}
{%- set repo_rev.setuptools_odoo = "3.1.8" %}
{%- endif %}

{%- if odoo_version > 16 %}
{%- set repo_rev.pylint_odoo = "v9.0.4" %}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing this v17 will have pylint_odoo = "v8.0.19" instead of "v9.0.4"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{%- if odoo_version == 17 %}
{%- set repo_rev.pylint_odoo = "v9.0.4" %}
{%- endif %}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, miss interpreted the conditions. So if v9.0.4 only applies to 17, we shouldn't remove the condition but instead modify to check for 17 only. In the previous state, the v9.1.2 for v17 and v18 were changed back to v9.0.4.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tihs is indeed easy to miss. I'd prefer adding a whole new section for v17, even if all the versions are identical to 16. So it is systematic, and easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new section for v17 was added

@sanderlienaerts sanderlienaerts force-pushed the cf-pre-commit-update-pylint-odoo-to-enable-support-for-v18 branch from a32f783 to 7b08994 Compare October 11, 2024 13:07
@sbidoul
Copy link
Member

sbidoul commented Oct 12, 2024

pylint-odoo unfortunately doesn't notify the user of 18.0 not being supported

TIL. I'm adding a new checkbox in OCA/maintainer-tools#628, so we don't forget next time.

@sanderlienaerts sanderlienaerts force-pushed the cf-pre-commit-update-pylint-odoo-to-enable-support-for-v18 branch from 7b08994 to c3fc659 Compare October 14, 2024 07:05
@sbidoul
Copy link
Member

sbidoul commented Oct 14, 2024

Thanks! Can you use the same versions in the 16 and 17 sections (at first sight it looks like you cloned the 18 section for 17) ?

@rven
Copy link

rven commented Oct 14, 2024

Thanks! Can you use the same versions in the 16 and 17 sections (at first sight it looks like you cloned the 18 section for 17) ?

I think it's correct what has been done here.

Originally there is a part <16 and a part <18 (for v16 and v17).
The last one is split up into <17 (v16) and <18 (v17).

@sbidoul
Copy link
Member

sbidoul commented Oct 14, 2024

Ah, yes, it looks ok, now. Either I was review a previous version or I misread.

Could you rebase to pickup the changes from #278?

@sanderlienaerts sanderlienaerts force-pushed the cf-pre-commit-update-pylint-odoo-to-enable-support-for-v18 branch from c3fc659 to 19974a2 Compare October 14, 2024 17:23
@sanderlienaerts
Copy link
Contributor Author

Changes are incorporated from origin/master

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sbidoul sbidoul disabled auto-merge October 15, 2024 04:46
@sbidoul sbidoul merged commit c17e82c into OCA:master Oct 15, 2024
7 checks passed
@sanderlienaerts sanderlienaerts deleted the cf-pre-commit-update-pylint-odoo-to-enable-support-for-v18 branch October 15, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants