-
Notifications
You must be signed in to change notification settings - Fork 88
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
build(pylint-odoo): bump for v16 #198
Conversation
Test are failing, (or are cancelled). could you check ? |
Fixes the problem explained in OCA/report-print-send#328 (comment).
El mar, 30 de may de 2023 a las 01:50:06 AM, Sylvain LE GAL
***@***.***> escribió:
Test are failing, (or are cancelled). could you check ?
Let's see now.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
Can we do that major version bump of pylint-odoo without risk? Are there not some checks that are removed from pylint-odoo 8 and moved to another project (https://github.com/OCA/odoo-pre-commit-hooks/)? |
@moylop260 told me to do it, and he maintains the project, so I guess he knows what he says 😉 But well, friend could you please answer @sbidoul's doubt? Thanks! |
This PR has the |
Amy news? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for @moylop260 answer, regarding @sbidoul remark that is legit.
@moylop260 can you check? |
pylint-odoo >= 8.0 includes OCA/pylint-odoo#396 which drops a lot of checks that have been 'migrated to https://github.com/OCA/odoo-pre-commit-hooks'. I can't find where this new linter is added to the OCA config. Do you have any idea? |
@StefanRijnhart these new checks are not in this template yet AFAIK. If we bump pylint-odoo to >=8, we need to add odoo-pre-commit-hooks to our .precommit-config.yaml, but we may also need to configure it to make sure we don't add new checks that would make existing branches red. |
Tests fail because we lost support for Python 2.7
|
Added. |
Any news? i d like to implement OCA/report-print-send#328 |
Can we provide any assistance to make it merged? |
I think we want to be reasonably sure that no new checks are added. In my imagination, that implies checking out a large number of OCA repos locally, apply this change and run the pre-commit to get an idea of the impact. Let me see if I can get oca-clone-everything working again. |
I ran this new pre-commit on all 16.0 OCA branches and I did not spot a large number of issues raised by it. In my comment above I suggested to adjust the indentation in the jinja template but I noticed that the template still leaves an empty line in the yaml configuration file that pre-commit will want to clean up anyway. That makes me less interested in fixing the indentation. If anybody wants to double check, here is the output of the pre-commit run: |
Co-authored-by: Stefan Rijnhart (Opener) <stefan@opener.am>
FWIW you don't have to be so much afraid of deploying changes to the template. Remember: it's not like in the old days. Repo per repo, branch per branch, once the template is updated, if anything is violating a new linter, it will raise a ❌. It's not like we're deploying a massive change by merging this. |
Is the CI failing something expected? |
@yajo Well, I liked to have this confirmed before I took the responsability and approved this change. @pedrobaeza Yes, Python 2.7 is gone. An attempt to revive it is #199. |
But seems to be not enough. Should we disable such CI? |
I have removed the requirement for py2.7 job to allow unblocking this situation until properly fixed. |
tests ok, i guess should be merged, correct ? |
It seems the test is still red. |
The First test Is Red because there Is not support Fort python 2.7, i Guess It Is ok |
@pedrobaeza Appreciating that there is already an initiative to get this test green again I think keeping it in its current red state is fine. |
I understood that @yajo remove such red. Should we merge with the red condition then? |
@yajo removed the |
OK, I merge then. |
I removed the python 2.7 from the requirements to merge PRs, that's why you were able to merge 😉 |
Hello, |
Updated report-print-send template |
Fixes the problem explained in OCA/report-print-send#328 (comment).