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

[IMP] Update pre-commit to current OCA standards, dob compatible #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dreispt
Copy link

@dreispt dreispt commented Apr 21, 2022

No description provided.

@dreispt
Copy link
Author

dreispt commented Apr 21, 2022

cc @fkantelberg

Copy link
Member

@fkantelberg fkantelberg left a comment

Choose a reason for hiding this comment

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

I have to test it but some of these changes seem breaking. This is a general template for all kind of Odoo versions. The update for the pre-commit should be tested as well.

odoo/.pylintrc Outdated
manifest_required_keys=license
manifest_deprecated_keys=description,active
license_allowed=AGPL-3,GPL-2,GPL-2 or any later version,GPL-3,GPL-3 or any later version,LGPL-3
valid_odoo_versions=15.0
Copy link
Member

Choose a reason for hiding this comment

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

We can't copy the file from the OCA 1 by 1. Internal manifests don't need to include OCA as author and the valid odoo version isn't always 15.0 because the template is used for all kind of Odoo versions.

manifest_required_keys=license
manifest_deprecated_keys=description,active
license_allowed=AGPL-3,GPL-2,GPL-2 or any later version,GPL-3,GPL-3 or any later version,LGPL-3
valid_odoo_versions=15.0
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@fkantelberg fkantelberg left a comment

Choose a reason for hiding this comment

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

I was able to test it a bit deeper. The additional pylint tests are quite helpful but I guess the readme shouldn't be required for this project because only internal modules are checked.

Additionally the black repinning causes heavy issues.

invalid-commit,
missing-manifest-dependency,
missing-newline-extrafiles,
missing-readme,
Copy link
Member

Choose a reason for hiding this comment

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

This one is duplicated and breaking because we don't want to enforce readme's for internal modules. Potentially some of the deprecated warnings can break older installations.

invalid-commit,
manifest-maintainers-list,
missing-newline-extrafiles,
missing-readme,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

- repo: https://github.com/psf/black
rev: 20.8b1
rev: 21.9b0
Copy link
Member

Choose a reason for hiding this comment

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

This black version has currently heavy problems with the current click version. I guess some of the CI for the OCA might be also influenced by this.

Traceback (most recent call last):
  File "/home/florian/.cache/pre-commit/repod82ojeyu/py_env-python3/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "/home/florian/.cache/pre-commit/repod82ojeyu/py_env-python3/lib/python3.8/site-packages/black/__init__.py", line 1282, in patched_main
    patch_click()
  File "/home/florian/.cache/pre-commit/repod82ojeyu/py_env-python3/lib/python3.8/site-packages/black/__init__.py", line 1268, in patch_click
    from click import _unicodefun  # type: ignore
ImportError: cannot import name '_unicodefun' from 'click' (/home/florian/.cache/pre-commit/repod82ojeyu/py_env-python3/lib/python3.8/site-packages/click/__init__.py)

psf/black#2964

Choose a reason for hiding this comment

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

What about this @dreispt @azoellner @fkantelberg is that stale or can this be fixed and merged somewhen soon

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