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

Prepare pre-commit-config for odoo v14 #8

Merged
merged 4 commits into from
Sep 30, 2020
Merged

Conversation

yajo
Copy link
Member

@yajo yajo commented Sep 23, 2020

  • Move linters after formatters. It makes no sense to lint code that is going to be reformatted.
  • Store revisions in variables and use them.
  • Where there are more changes than just the revisions, add needed logic.
  • Update dependencies.
  • Add v14 to CI.

@Tecnativa TT22772

@yajo yajo mentioned this pull request Sep 23, 2020
6 tasks
@yajo yajo self-assigned this Sep 23, 2020
@yajo yajo added the enhancement New feature or request label Sep 23, 2020
- Move linters after formatters. It makes no sense to lint code that is going to be reformatted.
- Store revisions in variables and use them.
- Where there are more changes than just the revisions, add needed logic.
- Update dependencies.
- Add v14 to CI.
@yajo
Copy link
Member Author

yajo commented Sep 29, 2020

One little thing regarding the pre-commit stuff: if there are plans for fixing OCA/oca-github-bot#55 some day, different pre-commit hook versions across module branches would dificult that a lot.

For example, code formatted with black 19.10b0 and 20.8b1 is quite different, so doing automatic fw/bw ports becomes much harder.

So, maybe we need to reconsider the decision of freezing hooks per odoo version, and, instead, update all of them at once, regardless of odoo version (in this context, this means branches 13.0 and later only).

The obvious problem of that decision would be that some open PRs that were green before the pre-commit update could become ❌ once merged. That has 2 possible solutions:

  1. Make the merge bot apply pre-commit if needed.
  2. Ask the OP to rebase and update the code (assuming some will get angry).

This PR is currently implementing the current decision of pre-commit hook per odoo version, and I'm not saying it's the right moment to change that. It's not a decision that we have to take right now because OCA/oca-github-bot#55 is far from fixed, but I'm just commenting it because we should keep this in mind.

@yajo yajo requested a review from lmignon September 30, 2020 08:19
@yajo
Copy link
Member Author

yajo commented Sep 30, 2020

This should be ready to squash and merge. May you review again please @lmignon and @sbidoul?


{#- Older versions that differ a lot have their own hardcoded templates for readability #}
{%- if odoo_version < 14 %}
{%- include "version-specific/%s/.pre-commit-config.yaml" % odoo_version %}
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you @yajo for all this hard work.

@OCA-git-bot
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@yajo
Copy link
Member Author

yajo commented Sep 30, 2020

Thank you too, merging then. 😊

@yajo yajo merged commit c383d11 into OCA:master Sep 30, 2020
@yajo yajo deleted the pre-commit-v14 branch September 30, 2020 10:34
@yajo yajo added this to the v0.1.0 milestone Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants