[WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #117

sbidoul · 2022-02-12T15:49:30Z

Towards #103

carmenbianca

Mostly LGTM, but some comments.

carmenbianca · 2022-02-23T12:21:46Z

version-specific/mqt-compat/.flake8

+per-file-ignores=
+    __init__.py:F401


Does not match https://github.com/OCA/maintainer-quality-tools/blob/master/travis/cfg/travis_run_flake8.cfg#L9

This may be intentional, but flagging it.

Yes that can be removed because the old flake8 oca/maintainer-quality-tools is using does not support per-file-ignores.

carmenbianca · 2022-02-23T12:29:38Z

version-specific/mqt-compat/.pre-commit-config.yaml

+        args: ["https://github.com/OCA/{{ repo_slug }}"]
+  - repo: https://github.com/acsone/setuptools-odoo


A lot of checks are missing here compared to 13.0:

forbidden-files

autoflake

black

prettier

pre-commit-hooks

pyupgrade

isort

Some of these exclusions seem intentional, but I'm not sure all of them are.

That is intentional. My goal is to imitate what oca/mqt does, so we create as little disruption as possible when we are going to apply this to 10,11,12 in order to shut down travis.

carmenbianca · 2022-02-23T12:43:05Z

version-specific/mqt-compat/.pylintrc.jinja

@@ -0,0 +1,100 @@
+[MASTER]


It'd probably be nice to use the same {% extends "src/.pylintrc-mandatory.jinja" %} mechanism here as in src/.

Proposal in coopiteasy@9ef6115

I've no strong opinion on this. As long as this does the same as the pylint configs of oca/maintainer-quality-tools.

It does basically the same, except .pylintrc now also repeats the .pylintrc-mandatory tests, which I think is desirable for local development.

carmenbianca · 2022-02-23T16:51:38Z

In OCA/product-variant#248 and in local testing flake8 has been, well, flaky. This error keeps popping up:

multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.7/x64/lib/python3.9/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/opt/hostedtoolcache/Python/3.9.7/x64/lib/python3.9/multiprocessing/pool.py", line 48, in mapstar
    return list(map(*args))
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/flake8/checker.py", line 646, in _run_checks
    return checker.run_checks()
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/flake8/checker.py", line 577, in run_checks
    self.run_ast_checks()
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/flake8/checker.py", line 484, in run_ast_checks
    checker = self.run_check(plugin, tree=ast)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/flake8/checker.py", line 433, in run_check
    return plugin['plugin'](**arguments)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/flake8/plugins/pyflakes.py", line 90, in __init__
    super(FlakesChecker, self).__init__(tree, filename,
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 494, in __init__
    self.handleChildren(tree)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 816, in handleChildren
    self.handleNode(node, tree)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 863, in handleNode
    handler(node)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 1208, in CLASSDEF
    self.handleNode(stmt, node)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 863, in handleNode
    handler(node)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 816, in handleChildren
    self.handleNode(node, tree)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 862, in handleNode
    handler = self.getNodeHandler(node.__class__)
  File "/home/runner/.cache/pre-commit/repo8npub7kc/py_env-python3/lib/python3.9/site-packages/pyflakes/checker.py", line 696, in getNodeHandler
    self._nodeHandlers[node_class] = handler = getattr(self, nodeType)
AttributeError: 'FlakesChecker' object has no attribute 'CONSTANT'
"""

pyflakes bug report found here: PyCQA/pyflakes#367

Fixed in pyflakes==2.1.0 (2019-01-23). I'm not sure what the direct relation between pyflakes and flake8 is, but I suspect that any 2019 release of flake8 should not exhibit this problem. (That, or bump down the Python version for the flake8 test.)

Any reason why there's an old version of flake8 (v3.4.1, 2017-07-28) in this PR?

sbidoul · 2022-02-23T18:19:11Z

Any reason why there's an old version of flake8 (v3.4.1, 2017-07-28) in this PR?

Because that is the one that MQT uses. And when I tried to use a more recent one, there were new errors because of new/different checks in recent flake8 versions.

Maybe this can be solved by pinning pyflakes to a version of the same era here.

Or by disabling the new checks introduced by flake8 since then. Not sure what's best.

yajo

Looks very good.

sbidoul · 2022-02-24T08:42:44Z

@carmenbianca I have squashed my commits here. Feel free to do PRs to my branches, or to cherry-pick my commit in a new PR of yours.

carmenbianca · 2022-02-24T09:11:36Z

@sbidoul I'm afraid I don't have the correct rights to push to your branch. In any case that shouldn't halt progress for now :)

carmenbianca · 2022-02-24T09:30:16Z

Notes on the flake8 problem:

Lowering Python version in .pre-commit-config.yaml to python3.7 (or lower) → Would work, but would mean that the pre-commit step will fail for everyone who doesn't specifically have python3.7 available on their dev machine.
Lowering Python version in .github/workflows/pre-commit.yml to python3.7 (or lower) → Would work, but would mean that the above reported error would still occur locally for anyone who has pre-commit installed against Python ≥3.8.
Using flake8 3.4.1 with a more recent pyflakes version (2.1.0) → Untested, and probably won't even install without patching because of this line in flake8 3.4.1.
Using a later flake8 version and disabling new tests → Would work, but may introduce discrepancies with MQT.

Of these options, 2 and 4 seem the least painful. The reason 2 probably isn't that bad is because I suspect that people doing dev work on Odoo 12 probably do so on Python 3.6/3.7, so they should have the ability to install pre-commit against a lower Python version. But it'll still result in a lot of time wasted à la 'why doesn't this work' for local development.

sbidoul · 2022-02-24T09:47:37Z

In my personal case, 1. would work best. Because I do have several python versions installed on my machine, while my pre-commit install is global and with the latest python version I have.

carmenbianca · 2022-02-24T10:00:16Z

Did a little research on to-disable tests for flake8: the flake8/pyflakes change logs sadly aren't very illuminating. The pycodestyle change log is more useful.

On second thought @sbidoul you're probably right that option 1 isn't that bad. Can we reasonably 'force' devs who work on Odoo 12 to have Python 3.6 xor Python 3.7 (depending on which is chosen) installed?

carmenbianca · 2022-02-24T10:12:09Z

Ah, it appears option 1 is mutually inclusive with option 2 (but not the other way around). If you specify Python 3.6 in .pre-commit-config.yaml, that Python interpreter also needs to be available in the pre-commit GitHub action, or it won't run. See failing workflow in OCA/product-variant#248.

carmenbianca · 2022-02-24T10:15:55Z

version-specific/mqt-compat/.flake8

+# F999 pylint support this case with expected tests
+# W503 changed by W504 and OCA prefers allow both
+# E203: whitespace before ':' (black behaviour and not pep8 compliant)
+ignore = E123,E133,E226,E241,E242,F811,F601,W503,W504,E203


F999 doesn't exist. I think F601 is intended?

carmenbianca · 2022-02-24T14:38:15Z

Pushed a partial fix for the failing pre-commit test here: coopiteasy@97f10c9 (edit 2022-03-02: fixed this link because it was incorrect)

However, the solution causes prettier to fail on files like src/{% if odoo_version > 12 %}.prettierrc.yml{% endif %} because of prettier/prettier#12143

So I've also pushed coopiteasy@087d738 to fix that.

The template used is this commit: <coopiteasy/oca-addons-repo-template@d28f504> At the time of writing, the 12.0 template for OCA is not yet finished, see <OCA/oca-addons-repo-template#117>. It was used regardless, with some modifications. The modifications are the additions of several tests in pre-commit, and the bumping of flake8. These modifications are made because the source material of this repo used these tools. The bumping of flake8 naively fixes an unresolved problem from the linked PR. Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

The template used is this commit: <coopiteasy/oca-addons-repo-template@d28f504> At the time of writing, the 12.0 template for OCA is not yet finished, see <OCA/oca-addons-repo-template#117>. It was used regardless, with some modifications. The modifications are the additions of several tests in pre-commit, and the bumping of flake8. These modifications are made because the source material of this repo used these tools. The bumping of flake8 naively fixes an unresolved problem from the linked PR. Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

The template used is this commit: <coopiteasy/oca-addons-repo-template@d28f504> At the time of writing, the 12.0 template for OCA is not yet finished, see <OCA/oca-addons-repo-template#117>. It was used regardless, with some modifications. The modifications are the additions of several tests in pre-commit, and the bumping of flake8. These modifications are made because the source material of this repo used these tools. The bumping of flake8 naively fixes an unresolved problem from the linked PR. Signed-off-by: Carmen Bianca Bakker <carmen@coopiteasy.be>

sbidoul · 2022-03-16T16:34:59Z

@carmenbianca will you do another PR with your work on this ?

sbidoul · 2022-03-18T18:34:22Z

This continues in #132

sbidoul changed the title ~~[WIP] MQT pre-commit + GitHub actions for 10, 11, 12~~ [WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 Feb 13, 2022

sbidoul mentioned this pull request Feb 13, 2022

[WIP][10.0] test with GitHub actions and lint with pre-commit OCA/l10n-italy#2658

Closed

sbidoul mentioned this pull request Feb 23, 2022

Add support for 10, 11, 12 #103

Closed

4 tasks

carmenbianca reviewed Feb 23, 2022

View reviewed changes

carmenbianca mentioned this pull request Feb 23, 2022

[WIP][12.0] GitHub actions testing + pre-commit linting OCA/product-variant#248

Closed

carmenbianca mentioned this pull request Feb 23, 2022

[WIP][10.0] GitHub actions testing + pre-commit linting OCA/product-variant#249

Closed

yajo approved these changes Feb 24, 2022

View reviewed changes

[WIP] MQT pre-commit + GitHub actions for 10, 11, 12

885143b

sbidoul force-pushed the support-10-11-12 branch from 2078d01 to 885143b Compare February 24, 2022 08:42

carmenbianca reviewed Feb 24, 2022

View reviewed changes

carmenbianca mentioned this pull request Feb 24, 2022

DO NOT MERGE: Custom changes coopiteasy/oca-addons-repo-template#1

Open

carmenbianca mentioned this pull request Mar 1, 2022

[IMP] Apply oca-addons-repo-template to project OCA/cooperative#1

Merged

legalsylvain mentioned this pull request Mar 16, 2022

[IMP] eslint: bump to escmascript 2019 #122

Merged

carmenbianca mentioned this pull request Mar 17, 2022

MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #132

Merged

sbidoul closed this Mar 18, 2022

sbidoul deleted the support-10-11-12 branch March 18, 2022 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #117

[WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #117

sbidoul commented Feb 12, 2022

carmenbianca left a comment

carmenbianca Feb 23, 2022

sbidoul Feb 23, 2022

carmenbianca Feb 23, 2022

sbidoul Feb 23, 2022

carmenbianca Feb 23, 2022

carmenbianca Feb 23, 2022

sbidoul Feb 23, 2022

carmenbianca Feb 23, 2022

carmenbianca commented Feb 23, 2022

sbidoul commented Feb 23, 2022 •

edited

Loading

yajo left a comment

sbidoul commented Feb 24, 2022

carmenbianca commented Feb 24, 2022 •

edited

Loading

carmenbianca commented Feb 24, 2022

sbidoul commented Feb 24, 2022

carmenbianca commented Feb 24, 2022

carmenbianca commented Feb 24, 2022

carmenbianca Feb 24, 2022

carmenbianca commented Feb 24, 2022 •

edited

Loading

sbidoul commented Mar 16, 2022

sbidoul commented Mar 18, 2022

		args: ["https://github.com/OCA/{{ repo_slug }}"]
		- repo: https://github.com/acsone/setuptools-odoo

		per-file-ignores=
		__init__.py:F401

[WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #117

[WIP] MQT-compatible pre-commit + GitHub actions for 10, 11, 12 #117

Conversation

sbidoul commented Feb 12, 2022

carmenbianca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmenbianca commented Feb 23, 2022

sbidoul commented Feb 23, 2022 • edited Loading

yajo left a comment

Choose a reason for hiding this comment

sbidoul commented Feb 24, 2022

carmenbianca commented Feb 24, 2022 • edited Loading

carmenbianca commented Feb 24, 2022

sbidoul commented Feb 24, 2022

carmenbianca commented Feb 24, 2022

carmenbianca commented Feb 24, 2022

Choose a reason for hiding this comment

carmenbianca commented Feb 24, 2022 • edited Loading

sbidoul commented Mar 16, 2022

sbidoul commented Mar 18, 2022

sbidoul commented Feb 23, 2022 •

edited

Loading

carmenbianca commented Feb 24, 2022 •

edited

Loading

carmenbianca commented Feb 24, 2022 •

edited

Loading