-
-
Notifications
You must be signed in to change notification settings - Fork 354
[18.0][MIG] graphql_base: Migration to 18.0 #474
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
base: 18.0
Are you sure you want to change the base?
Conversation
Currently translated at 100.0% (1 of 1 strings) Translation: rest-framework-16.0/rest-framework-16.0-graphql_base Translate-URL: https://translation.odoo-community.org/projects/rest-framework-16-0/rest-framework-16-0-graphql_base/it/
This conflicts with the build system, as the project root directory is in sys.path when building and there is confusion with the stdlib types module.
|
/ocabot migration graphql_base |
|
Hi, can you fix the pre-commit issue? |
graphql_base/controllers/main.py
Outdated
| except HttpQueryError as e: | ||
| result = json_encode({"errors": [{"message": str(e)}]}) | ||
| headers = dict(e.headers) | ||
| headers = dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you change this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imlopes i have revert the changes
also i have update the graphiql lib to latest version
55bc0bc to
daaede4
Compare
graphql_base/views/graphiql.xml
Outdated
| <script src="//unpkg.com/react@18/umd/react.production.min.js" /> | ||
| <script | ||
| src="//cdn.jsdelivr.net/npm/react-dom@16.2.0/umd/react-dom.production.min.js" | ||
| src="//unpkg.com/react-dom@18/umd/react-dom.production.min.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changing the cdn, and why not using the same CDN for all parts?
In any case I'd prefer to do this in a separate PR as this is not related to the Odoo migration, and could be backported independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbidoul I agree with you , i have remove the commit and will create new PR for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any update on this migration? When can it be merged into 18.0 branch?
daaede4 to
d82b0c4
Compare
graphql_base/controllers/main.py
Outdated
| return response | ||
| except HttpQueryError as e: | ||
| result = json_encode({"errors": [{"message": str(e)}]}) | ||
| headers = dict(e.headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is still different from version 17. Any reason to change it?
|
Looks like we are close to landing this. The goal is that you add only two commits on top of the current history:
See the guidelines here: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-18.0#technical-method-to-migrate-a-module-from-170-to-180-branch |
No description provided.