Sitelet https://web.archive.org/web/20201106202316/https://github.com/python-gitlab/python-gitlab/pull/1036
Skip to content
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

Async and sync compatible wrapper #1036

Draft
wants to merge 49 commits into
base: master
from

Conversation

@vishes-shell
Copy link

@vishes-shell vishes-shell commented Mar 4, 2020 •

I've rebuild wrapper to support async and sync interface with support of httpx.

There is two gitlab clients: Gitlab and AsyncGitlab, initerfaces are the same, except most of the methods would return awaitable object in AsyncGitlab.

Related #1025

vishes-shell added 30 commits Feb 18, 2020
Make all functions async
Use httpx instead of httpx
Make decorators async to correctly wrap async methods
Change _http_auth on direct httpx.AsyncClient.auth property
Fix some errors that are met on the way to async
This possibly is temporary solution because httpx raises TypeError when
tries to init DataField with bool value
feat: httpx based python-gitlab
on_http_error should work with sync functions that return coroutines and
handle errors when coroutines will be awaited
Also provide async and sync interface to interact with GitlabList
Provide base of gitlab client along with implementations of sync and
async interface
Basic principle is that we won't use constructor to create object,
instead classmethod would be used that return either object or corotine
that returns object
vishes-shell added 13 commits Feb 29, 2020
From now on v4 object method would return coroutine or desired data
Implement decorator that deals with possible awaitable object and run
function that is passed as callback and return awaitable or data
Decorate all edge cases with postprocessing in v4 objects with
awaitable_postprocess
Since _update_attrs is some sort of postprocess then we decorated it so
Hence changed interface we now can explicitly user self._update_attrs
but be sure that we return it, since it's awaitable
feat(async): async and sync compatible wrapper
vishes-shell added 2 commits Mar 5, 2020
chore: provide docstrings for gitlab client implementations
@vishes-shell
Copy link
Author

@vishes-shell vishes-shell commented Mar 5, 2020 •

Here is some sort of notable changes:

  1. response_content no longer support chunk_size since httpx does not support this. (encode/httpx#394)
  2. gitlab clients are moved to client.py
  3. 3 implementations of gitlab clients: gitlab.BaseGitlab abstract implementation that share common methods and store all docsrings, gitlab.Gitlab is sync implementation with httpx.Client, gitlab.AsyncGitlab is async implementation with httpx.AsyncClient.
  4. on_http_error make nested since we can get coroutine and need to catch same error in awaiting response
  5. RESTObject must be created (with create()), not initialized, since arguments could be coroutines, and need to be awaited, and __init__ does not support that interface
  6. Everything should be returned, since it can be couroutin
  7. every postprocess of response should be a function and be decorated with gitlab.utils.awaitable_postprocess, as it's done with _update_attrs and other edge cases
  8. unit tests are run by pytest and run against async and sync client implementations
  9. since unit tests are run with async gitlab, tests are required to be async, but fetching result is done through fixture that knows which client is used and return value if that's sync version or return await value if that's async version.
  10. GitlabList support two versions of iteration
  11. requests are completely removed
@nejch
Copy link
Member

@nejch nejch commented Mar 8, 2020

1. `response_content` no longer support `chunk_size` since `httpx` does not support
    this. ([encode/httpx#394](https://github.com/encode/httpx/issues/394))

I just noticed something: chunk_size is a configurable argument in several public methods (for blobs/snippet content, export download, etc) so it's possible people are using it and this would be a breaking change, right? Since you're replacing it with aiter_bytes and using chunks there, is there any way to preserve backward compatibility (I haven't checked the implementation)? Or wait for the upstream issue to resolve?

3. 3 implementations of gitlab clients: `gitlab.BaseGitlab` abstract implementation that share common methods and store all docsrings, `gitlab.Gitlab` is sync implementation with `httpx.Client`, `gitlab.AsyncGitlab` is async implementation with `httpx.AsyncClient`.
9. since unit tests are run with async gitlab, tests are required to be async, but fetching result is done through fixture that knows which client is used and `return value` if that's _sync_ version or `return await value` if that's `async` version.

That's really cool :)

@max-wittig
Copy link
Member

@max-wittig max-wittig commented Mar 8, 2020

@vishes-shell Thanks for all the work that you're doing!

People could have been using chunk_size, by providing a different requests instance, right?
(Which is also not supported anymore then?)

But we have nothing about this officially in the docs, so I would say we can solve this by mentioning it in the release notes. We anyway need to bump the version to 3.X.

@nejch
Copy link
Member

@nejch nejch commented Mar 8, 2020

@max-wittig I tried earlier and at least in some cases it can even be just:

project = gl.projects.get(1)
export = project.exports.create({})
dl = export.download(chunk_size=512)

From what I see in the current solution by @vishes-shell this would still work and wouldn't break anything, the argument would just be ignored by response_content(). But if anyone relies on chunk_size for some specific reason, it might break their behavior. I'm not sure of all the use cases but it seems useful especially for project exports: https://stackoverflow.com/questions/46205586/why-to-use-iter-content-and-chunk-size-in-python-requests/46205745

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.