[16.0][MIG] base_binary_url_import: Migration to 16.0 #618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lehoangan
force-pushed
the
16.0-mig-base_binary_url_import
branch
8 times, most recently
from
0a2c0d2
to
8514e17
Compare
lehoangan
changed the title
lehoangan
force-pushed
the
16.0-mig-base_binary_url_import
branch
from
2b0632b
to
b96dcd1
Compare
StephaneMangin
approved these changes
Apr 17, 2023
| else: | ||
| raise UserError(_("Identifier %s is not an Integer or XMLID") % target_id) |
There was a problem hiding this comment.
Suggested change
| else: | |
| raise UserError(_("Identifier %s is not an Integer or XMLID") % target_id) | |
| raise UserError(_("Identifier %s is not an Integer or XMLID") % target_id) |
There was a problem hiding this comment.
I think we don't need to modify as your suggestion
leemannd
reviewed
Apr 24, 2023
| @@ -0,0 +1,4 @@ | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
There was a problem hiding this comment.
In the OCA we don't consider usefull to add license in __init__.py
leemannd
reviewed
Apr 24, 2023
| @@ -0,0 +1,4 @@ | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
|
|
|||
| # from . import tes_binary_url_import | |||
There was a problem hiding this comment.
I will remove it. I don't have this test in the module
Comment on lines
279
to
289
| def _import_content_from_url(self, request_session): | ||
| def get_filename(respone): | ||
| content_disposition = respone.headers.get("Content-Disposition") | ||
| if content_disposition: | ||
| param, options = werkzeug.http.parse_options_header(content_disposition) | ||
| if param == "attachment" and options.get("filename"): | ||
| return options.get("filename") | ||
|
|
||
| path = urlparse(respone.url).path | ||
| name = path[path.rfind("/") + 1 :] | ||
| return name |
There was a problem hiding this comment.
I don't understand why we would like to maintain such code when an external library is doing it for us 😕
There was a problem hiding this comment.
Hello @grindtildeath
rfc6266-parserdepends onLEPL, which is not compatible with python 3.6+:
import rfc6266_parser
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/rfc6266_parser.py", line 14, in <module>
from lepl import (
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/__init__.py", line 113, in <module>
from lepl.contrib.matchers import SmartSeparator2
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/contrib/matchers.py", line 41, in <module>
from lepl.matchers.derived import Optional
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/matchers/derived.py", line 38, in <module>
from lepl.matchers.combine import And, DepthFirst, BreadthFirst, \
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/matchers/combine.py", line 45, in <module>
from lepl.matchers.core import Literal
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/matchers/core.py", line 45, in <module>
from lepl.matchers.support import OperatorMatcher, coerce_, \
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/matchers/support.py", line 37, in <module>
from lepl.core.config import ParserMixin
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/core/config.py", line 40, in <module>
from lepl.stream.factory import DEFAULT_STREAM_FACTORY
File "/home/nils/.local/share/virtualenvs/venv-odoo16/lib/python3.10/site-packages/lepl/stream/factory.py", line 31, in <module>
from collections import Iterable
ImportError: cannot import name 'Iterable' from 'collections' (/home/nils/.pythonz/pythons/CPython-3.10.10/lib/python3.10/collections/__init__.py)
rfc6266-parserwas forked to make it compatible with python 3.6+ and published asuc-rfc6266-parser;- BUT the way to make it compatible with python 3.6+ was to drop
LEPLin favor ofwerkzeug, pinned to 1.0.1, which makes it incompatible with odoo, that also needs a specificwerkzeugversion - a PR to unpin
werkzeugexists, but the formatgit+https://github.com/genme/python-rfc6266-parser.git@4af2f324be019bdd6ca6543bc36d1d031ca609d5#egg=rfc6266-parseris not compatible with odoo'sexternal_dependenciesmanifest key; - as an alternative, we could have published our own fork on pypi, BUT at that point we just decided to drop the dependency in favor of a simple function based on
werkzeug - NOW that we're having a second look, we just noticed that there is another quite recent fork from the original
rfc6266that took the option of usingpyparsinginstead ofLEPL: it could do the job, we'll look into it
There was a problem hiding this comment.
@nilshamerlinck Thank you very much for the explanation. 👍
IMO we should better use a lib as long as it does the job, but if we cannot let's just keep the function in the module, but move it to a dedicated rfc6266_parser.py file outside of the models folder.
There was a problem hiding this comment.
pyrfc6266 does the job, so custom function has been removed; test module has been included also
lehoangan
force-pushed
the
16.0-mig-base_binary_url_import
branch
2 times, most recently
from
b22acd3
to
daf98a5
Compare
lehoangan
force-pushed
the
16.0-mig-base_binary_url_import
branch
from
daf98a5
to
0eb1997
Compare
gurneyalex
approved these changes
Jun 28, 2023
|
/ocabot merge nobump |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at b771eeb. Thanks a lot for contributing to OCA. ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dependency on old
rfc6266-parserremoved, and replaced by a simple function relying onwerkzeug