8000 refactor: move response_content into backend code by nejch · Pull Request #2488 · python-gitlab/python-gitlab · GitHub
[go: up one dir, main page]
Skip to content

refactor: move response_content into backend code #2488

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: move response_content into backend code #2488

Show file tree
Hide file tree
Changes from all commits
Commits
8000
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion gitlab/_backends/protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import abc
import sys
from typing import Any, Dict, Optional, Union
from typing import Any, Callable, Dict, Iterator, Optional, Union

import requests
from requests_toolbelt.multipart.encoder import MultipartEncoder # type: ignore
Expand All @@ -17,6 +17,17 @@ def __init__(self, response: requests.Response) -> None: ...


class Backend(Protocol):
@staticmethod
@abc.abstractmethod
def response_content(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it might be a good time to make the method require keyword args for all the args. I'm a fan of requiring keyword args 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would be a breaking change as we still expose it in utils as a non-private function. In that case I'd maybe drop the wrapper entirely and not just deprecate it, as then we don't need to worry about maintaining the signature twice (#2488 (comment)).

Might be worth grouping breaking changes and wait to merge them all together though for a major release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I didn't think it would be a breaking change if this is the new function and then the wrapper (with the deprecation) would call this one with keyword args.

response: requests.Response,
streamed: bool,
action: Optional[Callable[[bytes], None]],
chunk_size: int,
*,
iterator: bool,
) -> Optional[Union[bytes, Iterator[Any]]]: ...

@abc.abstractmethod
def http_request(
self,
Expand Down
39 changes: 38 additions & 1 deletion gitlab/_backends/requests_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
from __future__ import annotations

import dataclasses
from typing import Any, BinaryIO, Dict, Optional, TYPE_CHECKING, Union
from typing import (
Any,
BinaryIO,
Callable,
Dict,
Iterator,
Optional,
TYPE_CHECKING,
Union,
)

import requests
from requests import PreparedRequest
Expand Down Expand Up @@ -55,6 +64,11 @@ def __post_init__(self) -> None:
)


class _StdoutStream:
def __call__(self, chunk: Any) -> None:
print(chunk)


class RequestsResponse(protocol.BackendResponse):
def __init__(self, response: requests.Response) -> None:
self._response: requests.Response = response
Expand Down Expand Up @@ -126,6 +140,29 @@ def prepare_send_data(

return SendData(json=post_data, content_type="application/json")

@staticmethod
def response_content(
response: requests.Response,
streamed: bool,
action: Optional[Callable[[bytes], None]],
chunk_size: int,
*,
iterator: bool,
) -> Optional[Union[bytes, Iterator[Any]]]:
if iterator:
return response.iter_content(chunk_size=chunk_size)

if streamed is False:
return response.content

if action is None:
action = _StdoutStream()

for chunk in response.iter_content(chunk_size=chunk_size):
if chunk:
action(chunk)
return None

def http_request(
self,
method: str,
Expand Down
2 changes: 1 addition & 1 deletion gitlab/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ def download(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
37 changes: 9 additions & 28 deletions gitlab/utils.py
View file 6D40
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,9 @@
import traceback
import urllib.parse
import warnings
from typing import Any, Callable, Dict, Iterator, Literal, Optional, Tuple, Type, Union
from typing import Any, Dict, Iterator, Literal, Optional, Tuple, Type, Union

import requests

from gitlab import types


class _StdoutStream:
def __call__(self, chunk: Any) -> None:
print(chunk)
from gitlab import _backends, types


def get_content_type(content_type: Optional[str]) -> str:
Expand Down Expand Up @@ -50,26 +43,14 @@ def format(self, record: logging.LogRecord) -> str:


def response_content(
response: requests.Response,
streamed: bool,
action: Optional[Callable[[bytes], None]],
chunk_size: int,
*,
iterator: bool,
*args: Any, **kwargs: Any
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep the signature as before. Instead of using *args/**kwargs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial idea was to just leave a simple wrapper for backward compatibility but maybe we can just drop it (see #2488 (comment)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that this one would maintain the same exact signature as before, and then call the new function with keyword arguments.

But! It isn't that important and perfectly fine with me if you want to merge this in as is 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I misunderstood your comment I think, I thought you wanted to match the signatures. I'll add the changes!

) -> Optional[Union[bytes, Iterator[Any]]]:
if iterator:
return response.iter_content(chunk_size=chunk_size)

if streamed is False:
return response.content

if action is None:
action = _StdoutStream()

for chunk in response.iter_content(chunk_size=chunk_size):
if chunk:
action(chunk)
return None
warn(
"`utils.response_content()` is deprecated and will be removed in a future"
"version.\nUse the current backend's `response_content()` method instead.",
category=DeprecationWarning,
)
return _backends.DefaultBackend.response_content(*args, **kwargs)


def _transform_types(
Expand Down
5 changes: 2 additions & 3 deletions gitlab/v4/objects/artifacts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

from gitlab import cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject

__all__ = ["ProjectArtifact", "ProjectArtifactManager"]
Expand Down Expand Up @@ -90,7 +89,7 @@ def download(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down Expand Up @@ -142,6 +141,6 @@ def raw(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)
2 changes: 1 addition & 1 deletion gitlab/v4/objects/files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ def raw(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
7 changes: 3 additions & 4 deletions gitlab/v4/objects/jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

from gitlab import cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject
from gitlab.mixins import RefreshMixin, RetrieveMixin
from gitlab.types import ArrayAttribute
Expand Down Expand Up @@ -152,7 +151,7 @@ def artifacts(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down Expand Up @@ -195,7 +194,7 @@ def artifact(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down Expand Up @@ -236,7 +235,7 @@ def trace(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
3 changes: 1 addition & 2 deletions gitlab/v4/objects/packages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

from gitlab import cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject
from gitlab.mixins import DeleteMixin, GetMixin, ListMixin, ObjectDeleteMixin

Expand Down Expand Up @@ -166,7 +165,7 @@ def download(
result = self.gitlab.http_get(path, streamed=streamed, raw=True, **kwargs)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
2 changes: 1 addition & 1 deletion gitlab/v4/objects/projects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ def snapshot(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
4 changes: 2 additions & 2 deletions gitlab/v4/objects/repositories.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def repository_raw_blob(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down Expand Up @@ -247,7 +247,7 @@ def repository_archive(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
3 changes: 1 addition & 2 deletions gitlab/v4/objects/secure_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

from gitlab import cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject
from gitlab.mixins import NoUpdateMixin, ObjectDeleteMixin
from gitlab.types import FileAttribute, RequiredOptional
Expand Down Expand Up @@ -54,7 +53,7 @@ def download(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
5 changes: 2 additions & 3 deletions gitlab/v4/objects/snippets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

from gitlab import cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject, RESTObjectList
from gitlab.mixins import CRUDMixin, ObjectDeleteMixin, SaveMixin, UserAgentDetailMixin
from gitlab.types import RequiredOptional
Expand Down Expand Up @@ -61,7 +60,7 @@ def content(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down Expand Up @@ -154,7 +153,7 @@ def content(
)
if TYPE_CHECKING:
assert isinstance(result, requests.Response)
return utils.response_content(
return self.manager.gitlab._backend.response_content(
result, streamed, action, chunk_size, iterator=iterator
)

Expand Down
23 changes: 23 additions & 0 deletions tests/unit/test_backends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import requests
import responses

from gitlab import _backends


@responses.activate
def test_streamed_response_content_with_requests(capsys):
responses.add(
method="GET",
url="https://example.com",
status=200,
body="test",
content_type="application/octet-stream",
)

resp = requests.get("https://example.com", stream=True)
_backends.RequestsBackend.response_content(
resp, streamed=True, action=None, chunk_size=1024, iterator=False
)

captured = capsys.readouterr()
assert "test" in captured.out
21 changes: 0 additions & 21 deletions tests/unit/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import warnings

import pytest
import requests
import responses

from gitlab import types, utils

Expand All @@ -23,25 +21,6 @@ def test_get_content_type(content_type, expected_type):
assert parsed_type == expected_type


@responses.activate
def test_response_content(capsys):
responses.add(
method="GET",
url="https://example.com",
status=200,
body="test",
content_type="application/octet-stream",
)

resp = requests.get("https://example.com", stream=True)
utils.response_content(
resp, streamed=True, action=None, chunk_size=1024, iterator=False
)

captured = capsys.readouterr()
assert "test" in captured.out


class TestEncodedId:
def test_init_str(self):
obj = utils.EncodedId("Hello")
Expand Down
Loading
0