-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upgrade to usage of openapi-python-client 0.22.0 #62
base: master
Are you sure you want to change the base?
Conversation
* removed pytest-recording and vcrpy because they are not being used * updated based on the Dependabot alerts
Compatible with openapi-python-client version 0.22.0. Adding only those which are already used in this repo for version 0.10.7 so far.
Newly introduced templates in openapi-python-client 0.22.0
|
||
[tool.black] | ||
[tool.ruff] | ||
line-length = 120 |
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.
not sure if relevant but there's an inconsistency with ruff config
Line 32 in 4f6eca5
line-length = 88 |
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.
I will probably change that in the pyproject.toml, but it is probably not much relevant, the pyproject.toml there is simply generated to introduce the generated client, but I just use the code in the abstraction layer and not introduce it as a standalone package, so I just don't use the pyproject.toml at all. I can probably check the pyproject.toml template and either don't generate it, or provide just some minimal info and defer rest to the top level of the package. Wdyt?
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.
They have a pyproject_ruff
template that we could include and copy the config from the top level ruff.toml
https://github.com/openapi-generators/openapi-python-client/blob/main/openapi_python_client/templates/pyproject_ruff.toml.jinja
We don't expose the bindings
project outside of the osidb_bindings
so maybe can be removed safely, one less file that we need to update on each release
https://github.com/openapi-generators/openapi-python-client/tree/v0.22.0/openapi_python_client/templates/client.py.jinja | ||
#} | ||
{# CHANGE START (1) - use old style client from openapi-python-client 0.10.7 #} | ||
{# import ssl |
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.
We are keeping the new template commented and still using the old implementation so we can change it later right?
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.
Exactly! I would like to switch to complete usage of the httpx instead of requests/aiohttp, but I wanted to make this transfer step as minimal as possible, so I am keeping the old implementation and will switch to the new one in second step, which should be much easier.
* switch out black,isort and flake8 for ruff * update tox * reformat/lint/sort imports to comply with ruff settings
4f6eca5
to
2f7c8f4
Compare
), | ||
{% else %} | ||
{# CHANGE START (8) - use base URL from client #} | ||
"url": f"{client.base_url}{{ endpoint.path }}", |
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.
missing slash?
"url": f"{client.base_url}{{ endpoint.path }}", | |
"url": f"{client.base_url}/{{ endpoint.path }}", |
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.
Also I wonder what happens if the base_url
already has a trailing slash
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.
Good catch. I am trimming the trailing slashes in the session.py
so it is covered.
{# CHANGE START (17) - include client in passed arguments #} | ||
{# response = await client.get_async_httpx_client().request( | ||
**kwargs | ||
) #} |
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.
include client in passed arguments
probably this was left from copy/paste
field_dict["{{ property.name }}"] = {{ property.python_name }} | ||
{% endif %} | ||
{# {% endif %} #} |
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.
we kept the endif
but the if
itself was removed in the comment
This PR:
Partially addresses OSIDB-3773