Skip to content

refactor: decouple CLI from custom method arguments #1999

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

Closed
wants to merge 1 commit into from
Closed

refactor: decouple CLI from custom method arguments #1999

Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 24 additions & 1 deletion gitlab/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@
import pprint
import textwrap
from types import ModuleType
from typing import Any, Dict, Iterable, Optional, Type, Union
from typing import Any, Callable, Dict, Iterable, Optional, Type, Union

import gitlab
from gitlab import types as g_types
from gitlab.exceptions import GitlabParsingError
from gitlab.types import F

from .client import Gitlab, GitlabList

Expand Down Expand Up @@ -328,6 +329,28 @@ def total(self) -> Optional[int]:
return self._list.total


def custom_attrs(
required: tuple = (), optional: tuple = (), exclusive: tuple = ()
) -> Callable[[F], F]:
"""Decorates a custom method to add a RequiredOptional attribute.

Args:
required: A tuple of API attributes required in the custom method
optional: A tuple of API attributes optional in the custom method
exclusive: A tuple of mutually exclusive API attributes in the custom method
"""

def decorator(func: F) -> F:
setattr(
func,
"_custom_attrs",
g_types.RequiredOptional(required, optional, exclusive),
)
return func

return decorator


class RESTManager:
"""Base class for CRUD operations on objects.

Expand Down
28 changes: 13 additions & 15 deletions gitlab/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@
import re
import sys
from types import ModuleType
from typing import Any, Callable, cast, Dict, Optional, Tuple, Type, TypeVar, Union
from typing import Any, Callable, cast, Dict, Optional, Tuple, Type, Union

from requests.structures import CaseInsensitiveDict

import gitlab.config
from gitlab.base import RESTObject
from gitlab.types import F, RequiredOptional

# This regex is based on:
# https://github.com/jpvanhal/inflection/blob/master/inflection/__init__.py
Expand All @@ -43,24 +44,18 @@
custom_actions: Dict[str, Dict[str, Tuple[Tuple[str, ...], Tuple[str, ...], bool]]] = {}


# For an explanation of how these type-hints work see:
# https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators
#
# The goal here is that functions which get decorated will retain their types.
__F = TypeVar("__F", bound=Callable[..., Any])


def register_custom_action(
cls_names: Union[str, Tuple[str, ...]],
mandatory: Tuple[str, ...] = (),
optional: Tuple[str, ...] = (),
custom_action: Optional[str] = None,
) -> Callable[[__F], __F]:
def wrap(f: __F) -> __F:
) -> Callable[[F], F]:
def wrap(f: F) -> F:
@functools.wraps(f)
def wrapped_f(*args: Any, **kwargs: Any) -> Any:
return f(*args, **kwargs)

action = custom_action or f.__name__.replace("_", "-")
custom_attrs = getattr(f, "_custom_attrs", RequiredOptional())

# in_obj defines whether the method belongs to the obj or the manager
in_obj = True
if isinstance(cls_names, tuple):
Expand All @@ -76,10 +71,13 @@ def wrapped_f(*args: Any, **kwargs: Any) -> Any:
if final_name not in custom_actions:
custom_actions[final_name] = {}

action = custom_action or f.__name__.replace("_", "-")
custom_actions[final_name][action] = (mandatory, optional, in_obj)
custom_actions[final_name][action] = (
custom_attrs.required,
custom_attrs.optional,
in_obj,
)

return cast(__F, wrapped_f)
return cast(F, wrapped_f)

return wrap

Expand Down
17 changes: 6 additions & 11 deletions gitlab/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import functools
from typing import Any, Callable, cast, Optional, Type, TYPE_CHECKING, TypeVar, Union
from typing import Any, Callable, cast, Optional, Type, TYPE_CHECKING, Union

from gitlab.types import F


class GitlabError(Exception):
Expand Down Expand Up @@ -286,14 +288,7 @@ class GitlabUnfollowError(GitlabOperationError):
pass


# For an explanation of how these type-hints work see:
# https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators
#
# The goal here is that functions which get decorated will retain their types.
__F = TypeVar("__F", bound=Callable[..., Any])


def on_http_error(error: Type[Exception]) -> Callable[[__F], __F]:
def on_http_error(error: Type[Exception]) -> Callable[[F], F]:
"""Manage GitlabHttpError exceptions.

This decorator function can be used to catch GitlabHttpError exceptions
Expand All @@ -303,14 +298,14 @@ def on_http_error(error: Type[Exception]) -> Callable[[__F], __F]:
The exception type to raise -- must inherit from GitlabError
"""

def wrap(f: __F) -> __F:
def wrap(f: F) -> F:
@functools.wraps(f)
def wrapped_f(*args: Any, **kwargs: Any) -> Any:
try:
return f(*args, **kwargs)
except GitlabHttpError as e:
raise error(e.error_message, e.response_code, e.response_body) from e

return cast(__F, wrapped_f)
return cast(F, wrapped_f)

return wrap
16 changes: 8 additions & 8 deletions gitlab/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,9 +547,8 @@ class AccessRequestMixin(_RestObjectBase):
_updated_attrs: Dict[str, Any]
manager: base.RESTManager

@cli.register_custom_action(
("ProjectAccessRequest", "GroupAccessRequest"), (), ("access_level",)
)
@cli.register_custom_action(("ProjectAccessRequest", "GroupAccessRequest"))
@base.custom_attrs(optional=("access_level",))
@exc.on_http_error(exc.GitlabUpdateError)
def approve(
self, access_level: int = gitlab.const.DEVELOPER_ACCESS, **kwargs: Any
Expand Down Expand Up @@ -721,7 +720,8 @@ def time_stats(self, **kwargs: Any) -> Dict[str, Any]:
assert not isinstance(result, requests.Response)
return result

@cli.register_custom_action(("ProjectIssue", "ProjectMergeRequest"), ("duration",))
@cli.register_custom_action(("ProjectIssue", "ProjectMergeRequest"))
Copy link
Member

Choose a reason for hiding this comment

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

What happens if these two decorators are put in reverse order?

Does it work? My guess is no. If not, do we detect a problem? My guess is also no.

If my guesses are correct can we add a way to detect improper usage and make sure the unit tests or something else fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question.. the thing is that, as I said in the comment above #1999 (comment), @cli.register_custom_action is the very next thing we'd want to get rid of if we want to fully decouple the CLI from the client. :( So not sure we should invest time in that and then remove it right after. 🤔

My idea for that was, instead of manually assigning custom methods to classes at each method definition, simply collect all non-standard (create,..) public methods for each Manager and RESTObject in the CLI module, kind of how we already collect the classes for the CLI:
https://github.com/python-gitlab/python-gitlab/blob/main/gitlab/cli.py#L97

Let me check if I can do that in a clean way in a separate commit or follow-up PR, I'll mark this a draft until then.

Maybe it makes sense I open an issue with a mini roadmap to untangling the CLI from the API so we have clear steps on how to get there.

@base.custom_attrs(required=("duration",))
@exc.on_http_error(exc.GitlabTimeTrackingError)
def time_estimate(self, duration: str, **kwargs: Any) -> Dict[str, Any]:
"""Set an estimated time of work for the object.
Expand Down Expand Up @@ -759,7 +759,8 @@ def reset_time_estimate(self, **kwargs: Any) -> Dict[str, Any]:
assert not isinstance(result, requests.Response)
return result

@cli.register_custom_action(("ProjectIssue", "ProjectMergeRequest"), ("duration",))
@cli.register_custom_action(("ProjectIssue", "ProjectMergeRequest"))
@base.custom_attrs(required=("duration",))
@exc.on_http_error(exc.GitlabTimeTrackingError)
def add_spent_time(self, duration: str, **kwargs: Any) -> Dict[str, Any]:
"""Add time spent working on the object.
Expand Down Expand Up @@ -833,9 +834,8 @@ def participants(self, **kwargs: Any) -> Dict[str, Any]:


class BadgeRenderMixin(_RestManagerBase):
@cli.register_custom_action(
("GroupBadgeManager", "ProjectBadgeManager"), ("link_url", "image_url")
)
@cli.register_custom_action(("GroupBadgeManager", "ProjectBadgeManager"))
@base.custom_attrs(required=("link_url", "image_url"))
@exc.on_http_error(exc.GitlabRenderError)
def render(self, link_url: str, image_url: str, **kwargs: Any) -> Dict[str, Any]:
"""Preview link_url and image_url after interpolation.
Expand Down
6 changes: 5 additions & 1 deletion gitlab/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import dataclasses
from typing import Any, Dict, List, Optional, Tuple, TYPE_CHECKING
from typing import Any, Callable, Dict, List, Optional, Tuple, TYPE_CHECKING, TypeVar

# TypeVar for decorators so that decorated functions retain their signatures.
# See https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators
F = TypeVar("F", bound=Callable[..., Any])


@dataclasses.dataclass(frozen=True)
Expand Down
17 changes: 7 additions & 10 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,7 @@
from gitlab import cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject
from gitlab.base import custom_attrs, RESTManager, RESTObject

__all__ = ["ProjectArtifact", "ProjectArtifactManager"]

Expand All @@ -25,9 +25,8 @@ class ProjectArtifactManager(RESTManager):
_path = "/projects/{project_id}/jobs/artifacts"
_from_parent_attrs = {"project_id": "id"}

@cli.register_custom_action(
"Project", ("ref_name", "job"), ("job_token",), custom_action="artifacts"
)
@cli.register_custom_action("Project", custom_action="artifacts")
@custom_attrs(required=("ref_name", "job"), optional=("job_token",))
def __call__(
self,
*args: Any,
Expand Down Expand Up @@ -62,9 +61,8 @@ def delete(self, **kwargs: Any) -> None:
assert path is not None
self.gitlab.http_delete(path, **kwargs)

@cli.register_custom_action(
"ProjectArtifactManager", ("ref_name", "job"), ("job_token",)
)
@cli.register_custom_action("ProjectArtifactManager")
@custom_attrs(required=("ref_name", "job"), optional=("job_token",))
@exc.on_http_error(exc.GitlabGetError)
def download(
self,
Expand Down Expand Up @@ -105,9 +103,8 @@ def download(
assert isinstance(result, requests.Response)
return utils.response_content(result, streamed, action, chunk_size)

@cli.register_custom_action(
"ProjectArtifactManager", ("ref_name", "artifact_path", "job")
)
@cli.register_custom_action("ProjectArtifactManager")
@custom_attrs(required=("ref_name", "artifact_path", "job"))
@exc.on_http_error(exc.GitlabGetError)
def raw(
self,
Expand Down
11 changes: 7 additions & 4 deletions gitlab/v4/objects/commits.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import requests

import gitlab
from gitlab import cli
from gitlab import base, cli
from gitlab import exceptions as exc
from gitlab.base import RESTManager, RESTObject
from gitlab.mixins import CreateMixin, ListMixin, RefreshMixin, RetrieveMixin
Expand Down Expand Up @@ -46,7 +46,8 @@ def diff(self, **kwargs: Any) -> Union[gitlab.GitlabList, List[Dict[str, Any]]]:
path = f"{self.manager.path}/{self.encoded_id}/diff"
return self.manager.gitlab.http_list(path, **kwargs)

@cli.register_custom_action("ProjectCommit", ("branch",))
@cli.register_custom_action("ProjectCommit")
@base.custom_attrs(required=("branch",))
@exc.on_http_error(exc.GitlabCherryPickError)
def cherry_pick(self, branch: str, **kwargs: Any) -> None:
"""Cherry-pick a commit into a branch.
Expand All @@ -63,7 +64,8 @@ def cherry_pick(self, branch: str, **kwargs: Any) -> None:
post_data = {"branch": branch}
self.manager.gitlab.http_post(path, post_data=post_data, **kwargs)

@cli.register_custom_action("ProjectCommit", optional=("type",))
@cli.register_custom_action("ProjectCommit")
@base.custom_attrs(optional=("type",))
@exc.on_http_error(exc.GitlabGetError)
def refs(
self, type: str = "all", **kwargs: Any
Expand Down Expand Up @@ -105,7 +107,8 @@ def merge_requests(
path = f"{self.manager.path}/{self.encoded_id}/merge_requests"
return self.manager.gitlab.http_list(path, **kwargs)

@cli.register_custom_action("ProjectCommit", ("branch",))
@cli.register_custom_action("ProjectCommit")
@base.custom_attrs(required=("branch",))
@exc.on_http_error(exc.GitlabRevertError)
def revert(
self, branch: str, **kwargs: Any
Expand Down
8 changes: 4 additions & 4 deletions gitlab/v4/objects/container_registry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from typing import Any, cast, TYPE_CHECKING, Union

from gitlab import cli
from gitlab import base, cli
from gitlab import exceptions as exc
from gitlab.base import RESTManager, RESTObject
from gitlab.mixins import DeleteMixin, ListMixin, ObjectDeleteMixin, RetrieveMixin
Expand Down Expand Up @@ -32,9 +32,9 @@ class ProjectRegistryTagManager(DeleteMixin, RetrieveMixin, RESTManager):
_from_parent_attrs = {"project_id": "project_id", "repository_id": "id"}
_path = "/projects/{project_id}/registry/repositories/{repository_id}/tags"

@cli.register_custom_action(
"ProjectRegistryTagManager",
("name_regex_delete",),
@cli.register_custom_action("ProjectRegistryTagManager")
@base.custom_attrs(
required=("name_regex_delete",),
optional=("keep_n", "name_regex_keep", "older_than"),
)
@exc.on_http_error(exc.GitlabDeleteError)
Expand Down
5 changes: 3 additions & 2 deletions gitlab/v4/objects/deploy_keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import requests

from gitlab import cli
from gitlab import base, cli
from gitlab import exceptions as exc
from gitlab.base import RESTManager, RESTObject
from gitlab.mixins import CRUDMixin, ListMixin, ObjectDeleteMixin, SaveMixin
Expand Down Expand Up @@ -36,7 +36,8 @@ class ProjectKeyManager(CRUDMixin, RESTManager):
_create_attrs = RequiredOptional(required=("title", "key"), optional=("can_push",))
_update_attrs = RequiredOptional(optional=("title", "can_push"))

@cli.register_custom_action("ProjectKeyManager", ("key_id",))
@cli.register_custom_action("ProjectKeyManager")
@base.custom_attrs(required=("key_id",))
@exc.on_http_error(exc.GitlabProjectDeployKeyError)
def enable(
self, key_id: int, **kwargs: Any
Expand Down
24 changes: 13 additions & 11 deletions gitlab/v4/objects/files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import requests

from gitlab import cli
from gitlab import base, cli
from gitlab import exceptions as exc
from gitlab import utils
from gitlab.base import RESTManager, RESTObject
Expand Down Expand Up @@ -98,7 +98,8 @@ class ProjectFileManager(GetMixin, CreateMixin, UpdateMixin, DeleteMixin, RESTMa
optional=("encoding", "author_email", "author_name"),
)

@cli.register_custom_action("ProjectFileManager", ("file_path", "ref"))
@cli.register_custom_action("ProjectFileManager")
@base.custom_attrs(required=("file_path", "ref"))
# NOTE(jlvillal): Signature doesn't match UpdateMixin.update() so ignore
# type error
def get( # type: ignore
Expand All @@ -120,10 +121,10 @@ def get( # type: ignore
"""
return cast(ProjectFile, GetMixin.get(self, file_path, ref=ref, **kwargs))

@cli.register_custom_action(
"ProjectFileManager",
("file_path", "branch", "content", "commit_message"),
("encoding", "author_email", "author_name"),
@cli.register_custom_action("ProjectFileManager")
@base.custom_attrs(
required=("file_path", "branch", "content", "commit_message"),
optional=("encoding", "author_email", "author_name"),
)
@exc.on_http_error(exc.GitlabCreateError)
def create(
Expand Down Expand Up @@ -187,9 +188,8 @@ def update( # type: ignore
assert isinstance(result, dict)
return result

@cli.register_custom_action(
"ProjectFileManager", ("file_path", "branch", "commit_message")
)
@cli.register_custom_action("ProjectFileManager")
@base.custom_attrs(required=("file_path", "branch", "commit_message"))
@exc.on_http_error(exc.GitlabDeleteError)
# NOTE(jlvillal): Signature doesn't match DeleteMixin.delete() so ignore
# type error
Expand All @@ -213,7 +213,8 @@ def delete( # type: ignore
data = {"branch": branch, "commit_message": commit_message}
self.gitlab.http_delete(path, query_data=data, **kwargs)

@cli.register_custom_action("ProjectFileManager", ("file_path", "ref"))
@cli.register_custom_action("ProjectFileManager")
@base.custom_attrs(required=("file_path", "ref"))
@exc.on_http_error(exc.GitlabGetError)
def raw(
self,
Expand Down Expand Up @@ -254,7 +255,8 @@ def raw(
assert isinstance(result, requests.Response)
return utils.response_content(result, streamed, action, chunk_size)

@cli.register_custom_action("ProjectFileManager", ("file_path", "ref"))
@cli.register_custom_action("ProjectFileManager")
@base.custom_attrs(required=("file_path", "ref"))
@exc.on_http_error(exc.GitlabListError)
def blame(self, file_path: str, ref: str, **kwargs: Any) -> List[Dict[str, Any]]:
"""Return the content of a file for a commit.
Expand Down
Loading