[FIX] l10n_fr_siret[_lookup]: remove wrong dependency #515
Conversation
|
Hi @alexis-via, @remi-filament, |
| @@ -14,7 +14,6 @@ | |||
| "depends": [ | |||
| "l10n_fr_siret", | |||
| ], | |||
| "external_dependencies": {"python": ["requests", "python-stdnum>=1.18"]}, | |||
There was a problem hiding this comment.
It's okay to specify direct dependencies here, if only for docs purposes. So the line must not be removed.
Maybe there a good reason to require python-stdnum>=1.18 for this module to work? In such case we must accept it.
There was a problem hiding this comment.
So the line must not be removed.
Restored
There was a problem hiding this comment.
Hi @yajo thank you for looking into this,
as a matter of fact there is a good reason to request python-stdnum >= 1.18, which is this commit : arthurdejong/python-stdnum@73f5e3a
The module should work with older version, although it will fail for the specific SIRET covered by the above commit (but this specific case is not tested in the module so unit tests should not fail neither if done with older version)
yajo
force-pushed
the
16.0-fix_python_stdnum_deps
branch
2 times, most recently
from
274a604
to
a9f1d65
Compare
| @@ -12,7 +12,7 @@ | |||
| "website": "https://github.com/OCA/l10n-france", | |||
| "license": "AGPL-3", | |||
| "depends": ["l10n_fr", "base_view_inheritance_extension"], | |||
| "external_dependencies": {"python": ["python-stdnum>=1.18"]}, | |||
| "external_dependencies": {"python": ["python-stdnum"]}, | |||
There was a problem hiding this comment.
You should keep it here also
| "external_dependencies": {"python": ["python-stdnum"]}, | |
| "external_dependencies": {"python": ["python-stdnum>=1.18"]}, |
| @@ -14,7 +14,6 @@ | |||
| "depends": [ | |||
| "l10n_fr_siret", | |||
| ], | |||
| "external_dependencies": {"python": ["requests", "python-stdnum>=1.18"]}, | |||
There was a problem hiding this comment.
Hi @yajo thank you for looking into this,
as a matter of fact there is a good reason to request python-stdnum >= 1.18, which is this commit : arthurdejong/python-stdnum@73f5e3a
The module should work with older version, although it will fail for the specific SIRET covered by the above commit (but this specific case is not tested in the module so unit tests should not fail neither if done with older version)
|
Yes, it's very annoying that Odoo depends on old version of python-stdnum. It's a lack of respect for the excellent work of the guys of python-stdnum. By depending on such an old version, we miss all the bugfixes of the recent years. |
|
I would not merge this. We'll find a better solution for OCA/oca-ci#66 |
|
What about this?
|
|
@yajo I'm ok to remove the constraint on the version of python-stdnum in the manifest and explain in the README of the module that we recommend to have a version >= 1.18 to have the fix on SIRET that is included in that version. |
yajo
force-pushed
the
16.0-fix_python_stdnum_deps
branch
from
a9f1d65
to
8c61202
Compare
|
seems fair, done. |
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
Odoo 16 depends on both requests and python-stdnum, so there's no need to specify those dependencies. Besides, [python-stdnum is pinned to 1.16][1]. Thus, when applying OCA/oca-ci#66 locally, the constraint failed. OCA addons should work with the dependencies pinned upstream. [1]: https://github.com/odoo/odoo/blob/fe4d8e8014e50d4335e0a8b052b5098f3a79bcec/requirements.txt#L48 @moduon MT-4520
yajo
force-pushed
the
16.0-fix_python_stdnum_deps
branch
from
8c61202
to
781a01e
Compare
|
Thanks, bot. AFAIK this is ready to merge. I just rebased to fix conflicts. |
github-actions
bot
removed
the
stale
Odoo 16 depends on both requests and python-stdnum, so there's no need to specify those dependencies.
Besides, python-stdnum is pinned to 1.16. Thus, when applying OCA/oca-ci#66 locally, the constraint failed. OCA addons should work with the dependencies pinned upstream.
@moduon MT-4520 @EmilioPascual