From 09b3b2225361722f2439952d2dbee6a48a9f9fd9 Mon Sep 17 00:00:00 2001 From: Nejc Habjan Date: Sun, 8 May 2022 01:14:52 +0200 Subject: [PATCH 001/935] refactor(mixins): extract custom type transforms into utils --- gitlab/mixins.py | 48 ++++------------------------------------ gitlab/utils.py | 37 ++++++++++++++++++++++++++++++- tests/unit/test_utils.py | 29 +++++++++++++++++++++++- 3 files changed, 68 insertions(+), 46 deletions(-) diff --git a/gitlab/mixins.py b/gitlab/mixins.py index 1a3ff4dbf..a29c7a782 100644 --- a/gitlab/mixins.py +++ b/gitlab/mixins.py @@ -33,7 +33,6 @@ import gitlab from gitlab import base, cli from gitlab import exceptions as exc -from gitlab import types as g_types from gitlab import utils __all__ = [ @@ -214,8 +213,8 @@ def list(self, **kwargs: Any) -> Union[base.RESTObjectList, List[base.RESTObject GitlabListError: If the server cannot perform the request """ - # Duplicate data to avoid messing with what the user sent us - data = kwargs.copy() + data, _ = utils._transform_types(kwargs, self._types, transform_files=False) + if self.gitlab.per_page: data.setdefault("per_page", self.gitlab.per_page) @@ -226,13 +225,6 @@ def list(self, **kwargs: Any) -> Union[base.RESTObjectList, List[base.RESTObject if self.gitlab.order_by: data.setdefault("order_by", self.gitlab.order_by) - # We get the attributes that need some special transformation - if self._types: - for attr_name, type_cls in self._types.items(): - if attr_name in data.keys(): - type_obj = type_cls(data[attr_name]) - data[attr_name] = type_obj.get_for_api() - # Allow to overwrite the path, handy for custom listings path = data.pop("path", self.path) @@ -298,23 +290,7 @@ def create( data = {} self._check_missing_create_attrs(data) - files = {} - - # We get the attributes that need some special transformation - if self._types: - # Duplicate data to avoid messing with what the user sent us - data = data.copy() - for attr_name, type_cls in self._types.items(): - if attr_name in data.keys(): - type_obj = type_cls(data[attr_name]) - - # if the type if FileAttribute we need to pass the data as - # file - if isinstance(type_obj, g_types.FileAttribute): - k = type_obj.get_file_name(attr_name) - files[attr_name] = (k, data.pop(attr_name)) - else: - data[attr_name] = type_obj.get_for_api() + data, files = utils._transform_types(data, self._types) # Handle specific URL for creation path = kwargs.pop("path", self.path) @@ -394,23 +370,7 @@ def update( path = f"{self.path}/{utils.EncodedId(id)}" self._check_missing_update_attrs(new_data) - files = {} - - # We get the attributes that need some special transformation - if self._types: - # Duplicate data to avoid messing with what the user sent us - new_data = new_data.copy() - for attr_name, type_cls in self._types.items(): - if attr_name in new_data.keys(): - type_obj = type_cls(new_data[attr_name]) - - # if the type if FileAttribute we need to pass the data as - # file - if isinstance(type_obj, g_types.FileAttribute): - k = type_obj.get_file_name(attr_name) - files[attr_name] = (k, new_data.pop(attr_name)) - else: - new_data[attr_name] = type_obj.get_for_api() + new_data, files = utils._transform_types(new_data, self._types) http_method = self._get_update_method() result = http_method(path, post_data=new_data, files=files, **kwargs) diff --git a/gitlab/utils.py b/gitlab/utils.py index 197935549..a05cb22fa 100644 --- a/gitlab/utils.py +++ b/gitlab/utils.py @@ -19,10 +19,12 @@ import traceback import urllib.parse import warnings -from typing import Any, Callable, Dict, Optional, Type, Union +from typing import Any, Callable, Dict, Optional, Tuple, Type, Union import requests +from gitlab import types + class _StdoutStream: def __call__(self, chunk: Any) -> None: @@ -47,6 +49,39 @@ def response_content( return None +def _transform_types( + data: Dict[str, Any], custom_types: dict, *, transform_files: Optional[bool] = True +) -> Tuple[dict, dict]: + """Copy the data dict with attributes that have custom types and transform them + before being sent to the server. + + If ``transform_files`` is ``True`` (default), also populates the ``files`` dict for + FileAttribute types with tuples to prepare fields for requests' MultipartEncoder: + https://toolbelt.readthedocs.io/en/latest/user.html#multipart-form-data-encoder + + Returns: + A tuple of the transformed data dict and files dict""" + + # Duplicate data to avoid messing with what the user sent us + data = data.copy() + files = {} + + for attr_name, type_cls in custom_types.items(): + if attr_name not in data: + continue + + type_obj = type_cls(data[attr_name]) + + # if the type if FileAttribute we need to pass the data as file + if transform_files and isinstance(type_obj, types.FileAttribute): + key = type_obj.get_file_name(attr_name) + files[attr_name] = (key, data.pop(attr_name)) + else: + data[attr_name] = type_obj.get_for_api() + + return data, files + + def copy_dict( *, src: Dict[str, Any], diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 7641c6979..3a92604bc 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -18,7 +18,7 @@ import json import warnings -from gitlab import utils +from gitlab import types, utils class TestEncodedId: @@ -95,3 +95,30 @@ def test_warn(self): assert warn_message in str(warning.message) assert __file__ in str(warning.message) assert warn_source == warning.source + + +def test_transform_types_copies_data_with_empty_files(): + data = {"attr": "spam"} + new_data, files = utils._transform_types(data, {}) + + assert new_data is not data + assert new_data == data + assert files == {} + + +def test_transform_types_with_transform_files_populates_files(): + custom_types = {"attr": types.FileAttribute} + data = {"attr": "spam"} + new_data, files = utils._transform_types(data, custom_types) + + assert new_data == {} + assert files["attr"] == ("attr", "spam") + + +def test_transform_types_without_transform_files_populates_data_with_empty_files(): + custom_types = {"attr": types.FileAttribute} + data = {"attr": "spam"} + new_data, files = utils._transform_types(data, custom_types, transform_files=False) + + assert new_data == {"attr": "spam"} + assert files == {} From de8c6e80af218d93ca167f8b5ff30319a2781d91 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sun, 29 May 2022 09:52:24 -0700 Subject: [PATCH 002/935] docs: use `as_list=False` or `all=True` in Getting started In the "Getting started with the API" section of the documentation, use either `as_list=False` or `all=True` in the example usages of the `list()` method. Also add a warning about the fact that `list()` by default does not return all items. --- docs/api-usage.rst | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/docs/api-usage.rst b/docs/api-usage.rst index 06c186cc9..b072d295d 100644 --- a/docs/api-usage.rst +++ b/docs/api-usage.rst @@ -93,13 +93,13 @@ Examples: .. code-block:: python # list all the projects - projects = gl.projects.list() + projects = gl.projects.list(as_list=False) for project in projects: print(project) # get the group with id == 2 group = gl.groups.get(2) - for project in group.projects.list(): + for project in group.projects.list(as_list=False): print(project) # create a new user @@ -107,6 +107,12 @@ Examples: user = gl.users.create(user_data) print(user) +.. warning:: + Calling ``list()`` without any arguments will by default not return the complete list + of items. Use either the ``all=True`` or ``as_list=False`` parameters to get all the + items when using listing methods. See the :ref:`pagination` section for more + information. + You can list the mandatory and optional attributes for object creation and update with the manager's ``get_create_attrs()`` and ``get_update_attrs()`` methods. They return 2 tuples, the first one is the list of mandatory @@ -133,7 +139,7 @@ Some objects also provide managers to access related GitLab resources: # list the issues for a project project = gl.projects.get(1) - issues = project.issues.list() + issues = project.issues.list(all=True) python-gitlab allows to send any data to the GitLab server when making queries. In case of invalid or missing arguments python-gitlab will raise an exception @@ -150,9 +156,9 @@ conflict with python or python-gitlab when using them as kwargs: .. code-block:: python - gl.user_activities.list(from='2019-01-01') ## invalid + gl.user_activities.list(from='2019-01-01', as_list=False) ## invalid - gl.user_activities.list(query_parameters={'from': '2019-01-01'}) # OK + gl.user_activities.list(query_parameters={'from': '2019-01-01'}, as_list=False) # OK Gitlab Objects ============== @@ -233,6 +239,8 @@ a project (the previous example used 2 API calls): project = gl.projects.get(1, lazy=True) # no API call project.star() # API call +.. _pagination: + Pagination ========== From cdc6605767316ea59e1e1b849683be7b3b99e0ae Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sun, 29 May 2022 15:50:19 -0700 Subject: [PATCH 003/935] feat(client): introduce `iterator=True` and deprecate `as_list=False` in `list()` `as_list=False` is confusing as it doesn't explain what is being returned. Replace it with `iterator=True` which more clearly explains to the user that an iterator/generator will be returned. This maintains backward compatibility with `as_list` but does issue a DeprecationWarning if `as_list` is set. --- docs/api-usage.rst | 22 +++++++++++------- docs/gl_objects/search.rst | 4 ++-- docs/gl_objects/users.rst | 2 +- gitlab/client.py | 31 +++++++++++++++++++------ gitlab/mixins.py | 6 ++--- gitlab/v4/objects/ldap.py | 4 ++-- gitlab/v4/objects/merge_requests.py | 8 ++----- gitlab/v4/objects/milestones.py | 16 ++++--------- gitlab/v4/objects/repositories.py | 4 ++-- gitlab/v4/objects/runners.py | 2 +- gitlab/v4/objects/users.py | 4 ++-- tests/functional/api/test_gitlab.py | 17 +++++++++++--- tests/functional/api/test_projects.py | 2 +- tests/unit/mixins/test_mixin_methods.py | 4 ++-- tests/unit/test_gitlab.py | 8 +++---- tests/unit/test_gitlab_http_methods.py | 28 ++++++++++++++++++---- 16 files changed, 99 insertions(+), 63 deletions(-) diff --git a/docs/api-usage.rst b/docs/api-usage.rst index b072d295d..aa6c4fe2c 100644 --- a/docs/api-usage.rst +++ b/docs/api-usage.rst @@ -93,13 +93,13 @@ Examples: .. code-block:: python # list all the projects - projects = gl.projects.list(as_list=False) + projects = gl.projects.list(iterator=True) for project in projects: print(project) # get the group with id == 2 group = gl.groups.get(2) - for project in group.projects.list(as_list=False): + for project in group.projects.list(iterator=True): print(project) # create a new user @@ -109,7 +109,7 @@ Examples: .. warning:: Calling ``list()`` without any arguments will by default not return the complete list - of items. Use either the ``all=True`` or ``as_list=False`` parameters to get all the + of items. Use either the ``all=True`` or ``iterator=True`` parameters to get all the items when using listing methods. See the :ref:`pagination` section for more information. @@ -156,9 +156,9 @@ conflict with python or python-gitlab when using them as kwargs: .. code-block:: python - gl.user_activities.list(from='2019-01-01', as_list=False) ## invalid + gl.user_activities.list(from='2019-01-01', iterator=True) ## invalid - gl.user_activities.list(query_parameters={'from': '2019-01-01'}, as_list=False) # OK + gl.user_activities.list(query_parameters={'from': '2019-01-01'}, iterator=True) # OK Gitlab Objects ============== @@ -282,13 +282,13 @@ order options. At the time of writing, only ``order_by="id"`` works. Reference: https://docs.gitlab.com/ce/api/README.html#keyset-based-pagination -``list()`` methods can also return a generator object which will handle the -next calls to the API when required. This is the recommended way to iterate -through a large number of items: +``list()`` methods can also return a generator object, by passing the argument +``iterator=True``, which will handle the next calls to the API when required. This +is the recommended way to iterate through a large number of items: .. code-block:: python - items = gl.groups.list(as_list=False) + items = gl.groups.list(iterator=True) for item in items: print(item.attributes) @@ -310,6 +310,10 @@ The generator exposes extra listing information as received from the server: For more information see: https://docs.gitlab.com/ee/user/gitlab_com/index.html#pagination-response-headers +.. note:: + Prior to python-gitlab 3.6.0 the argument ``as_list`` was used instead of + ``iterator``. ``as_list=False`` is the equivalent of ``iterator=True``. + Sudo ==== diff --git a/docs/gl_objects/search.rst b/docs/gl_objects/search.rst index 4030a531a..44773099d 100644 --- a/docs/gl_objects/search.rst +++ b/docs/gl_objects/search.rst @@ -63,13 +63,13 @@ The ``search()`` methods implement the pagination support:: # get a generator that will automatically make required API calls for # pagination - for item in gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, as_list=False): + for item in gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, iterator=True): do_something(item) The search API doesn't return objects, but dicts. If you need to act on objects, you need to create them explicitly:: - for item in gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, as_list=False): + for item in gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, iterator=True): issue_project = gl.projects.get(item['project_id'], lazy=True) issue = issue_project.issues.get(item['iid']) issue.state = 'closed' diff --git a/docs/gl_objects/users.rst b/docs/gl_objects/users.rst index 7a169dc43..01efefa7e 100644 --- a/docs/gl_objects/users.rst +++ b/docs/gl_objects/users.rst @@ -413,4 +413,4 @@ Get the users activities:: activities = gl.user_activities.list( query_parameters={'from': '2018-07-01'}, - all=True, as_list=False) + all=True, iterator=True) diff --git a/gitlab/client.py b/gitlab/client.py index b8ac22223..2ac5158f6 100644 --- a/gitlab/client.py +++ b/gitlab/client.py @@ -807,7 +807,9 @@ def http_list( self, path: str, query_data: Optional[Dict[str, Any]] = None, - as_list: Optional[bool] = None, + *, + as_list: Optional[bool] = None, # Deprecated in favor of `iterator` + iterator: Optional[bool] = None, **kwargs: Any, ) -> Union["GitlabList", List[Dict[str, Any]]]: """Make a GET request to the Gitlab server for list-oriented queries. @@ -816,12 +818,13 @@ def http_list( path: Path or full URL to query ('/projects' or 'http://whatever/v4/api/projects') query_data: Data to send as query parameters + iterator: Indicate if should return a generator (True) **kwargs: Extra options to send to the server (e.g. sudo, page, per_page) Returns: - A list of the objects returned by the server. If `as_list` is - False and no pagination-related arguments (`page`, `per_page`, + A list of the objects returned by the server. If `iterator` is + True and no pagination-related arguments (`page`, `per_page`, `all`) are defined then a GitlabList object (generator) is returned instead. This object will make API calls when needed to fetch the next items from the server. @@ -832,15 +835,29 @@ def http_list( """ query_data = query_data or {} - # In case we want to change the default behavior at some point - as_list = True if as_list is None else as_list + # Don't allow both `as_list` and `iterator` to be set. + if as_list is not None and iterator is not None: + raise ValueError( + "Only one of `as_list` or `iterator` can be used. " + "Use `iterator` instead of `as_list`. `as_list` is deprecated." + ) + + if as_list is not None: + iterator = not as_list + utils.warn( + message=( + f"`as_list={as_list}` is deprecated and will be removed in a " + f"future version. Use `iterator={iterator}` instead." + ), + category=DeprecationWarning, + ) get_all = kwargs.pop("all", None) url = self._build_url(path) page = kwargs.get("page") - if as_list is False: + if iterator: # Generator requested return GitlabList(self, url, query_data, **kwargs) @@ -879,7 +896,7 @@ def should_emit_warning() -> bool: utils.warn( message=( f"Calling a `list()` method without specifying `all=True` or " - f"`as_list=False` will return a maximum of {gl_list.per_page} items. " + f"`iterator=True` will return a maximum of {gl_list.per_page} items. " f"Your query returned {len(items)} of {total_items} items. See " f"{_PAGINATION_URL} for more details. If this was done intentionally, " f"then this warning can be supressed by adding the argument " diff --git a/gitlab/mixins.py b/gitlab/mixins.py index a29c7a782..850ce8103 100644 --- a/gitlab/mixins.py +++ b/gitlab/mixins.py @@ -201,12 +201,12 @@ def list(self, **kwargs: Any) -> Union[base.RESTObjectList, List[base.RESTObject all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is + iterator: If set to True and no pagination option is defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Returns: - The list of objects, or a generator if `as_list` is False + The list of objects, or a generator if `iterator` is True Raises: GitlabAuthenticationError: If authentication is not correct @@ -846,8 +846,6 @@ def participants(self, **kwargs: Any) -> Dict[str, Any]: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: diff --git a/gitlab/v4/objects/ldap.py b/gitlab/v4/objects/ldap.py index 10667b476..4a01061c5 100644 --- a/gitlab/v4/objects/ldap.py +++ b/gitlab/v4/objects/ldap.py @@ -26,12 +26,12 @@ def list(self, **kwargs: Any) -> Union[List[LDAPGroup], RESTObjectList]: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is + iterator: If set to True and no pagination option is defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Returns: - The list of objects, or a generator if `as_list` is False + The list of objects, or a generator if `iterator` is True Raises: GitlabAuthenticationError: If authentication is not correct diff --git a/gitlab/v4/objects/merge_requests.py b/gitlab/v4/objects/merge_requests.py index edd7d0195..a3c583bb5 100644 --- a/gitlab/v4/objects/merge_requests.py +++ b/gitlab/v4/objects/merge_requests.py @@ -199,8 +199,6 @@ def closes_issues(self, **kwargs: Any) -> RESTObjectList: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: @@ -211,7 +209,7 @@ def closes_issues(self, **kwargs: Any) -> RESTObjectList: List of issues """ path = f"{self.manager.path}/{self.encoded_id}/closes_issues" - data_list = self.manager.gitlab.http_list(path, as_list=False, **kwargs) + data_list = self.manager.gitlab.http_list(path, iterator=True, **kwargs) if TYPE_CHECKING: assert isinstance(data_list, gitlab.GitlabList) manager = ProjectIssueManager(self.manager.gitlab, parent=self.manager._parent) @@ -226,8 +224,6 @@ def commits(self, **kwargs: Any) -> RESTObjectList: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: @@ -239,7 +235,7 @@ def commits(self, **kwargs: Any) -> RESTObjectList: """ path = f"{self.manager.path}/{self.encoded_id}/commits" - data_list = self.manager.gitlab.http_list(path, as_list=False, **kwargs) + data_list = self.manager.gitlab.http_list(path, iterator=True, **kwargs) if TYPE_CHECKING: assert isinstance(data_list, gitlab.GitlabList) manager = ProjectCommitManager(self.manager.gitlab, parent=self.manager._parent) diff --git a/gitlab/v4/objects/milestones.py b/gitlab/v4/objects/milestones.py index e415330e4..0c4d74b59 100644 --- a/gitlab/v4/objects/milestones.py +++ b/gitlab/v4/objects/milestones.py @@ -33,8 +33,6 @@ def issues(self, **kwargs: Any) -> RESTObjectList: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: @@ -46,7 +44,7 @@ def issues(self, **kwargs: Any) -> RESTObjectList: """ path = f"{self.manager.path}/{self.encoded_id}/issues" - data_list = self.manager.gitlab.http_list(path, as_list=False, **kwargs) + data_list = self.manager.gitlab.http_list(path, iterator=True, **kwargs) if TYPE_CHECKING: assert isinstance(data_list, RESTObjectList) manager = GroupIssueManager(self.manager.gitlab, parent=self.manager._parent) @@ -62,8 +60,6 @@ def merge_requests(self, **kwargs: Any) -> RESTObjectList: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: @@ -74,7 +70,7 @@ def merge_requests(self, **kwargs: Any) -> RESTObjectList: The list of merge requests """ path = f"{self.manager.path}/{self.encoded_id}/merge_requests" - data_list = self.manager.gitlab.http_list(path, as_list=False, **kwargs) + data_list = self.manager.gitlab.http_list(path, iterator=True, **kwargs) if TYPE_CHECKING: assert isinstance(data_list, RESTObjectList) manager = GroupIssueManager(self.manager.gitlab, parent=self.manager._parent) @@ -114,8 +110,6 @@ def issues(self, **kwargs: Any) -> RESTObjectList: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: @@ -127,7 +121,7 @@ def issues(self, **kwargs: Any) -> RESTObjectList: """ path = f"{self.manager.path}/{self.encoded_id}/issues" - data_list = self.manager.gitlab.http_list(path, as_list=False, **kwargs) + data_list = self.manager.gitlab.http_list(path, iterator=True, **kwargs) if TYPE_CHECKING: assert isinstance(data_list, RESTObjectList) manager = ProjectIssueManager(self.manager.gitlab, parent=self.manager._parent) @@ -143,8 +137,6 @@ def merge_requests(self, **kwargs: Any) -> RESTObjectList: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is - defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Raises: @@ -155,7 +147,7 @@ def merge_requests(self, **kwargs: Any) -> RESTObjectList: The list of merge requests """ path = f"{self.manager.path}/{self.encoded_id}/merge_requests" - data_list = self.manager.gitlab.http_list(path, as_list=False, **kwargs) + data_list = self.manager.gitlab.http_list(path, iterator=True, **kwargs) if TYPE_CHECKING: assert isinstance(data_list, RESTObjectList) manager = ProjectMergeRequestManager( diff --git a/gitlab/v4/objects/repositories.py b/gitlab/v4/objects/repositories.py index f2792b14e..5826d9d83 100644 --- a/gitlab/v4/objects/repositories.py +++ b/gitlab/v4/objects/repositories.py @@ -60,7 +60,7 @@ def repository_tree( all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is + iterator: If set to True and no pagination option is defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) @@ -172,7 +172,7 @@ def repository_contributors( all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is + iterator: If set to True and no pagination option is defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) diff --git a/gitlab/v4/objects/runners.py b/gitlab/v4/objects/runners.py index 665e7431b..51f68611a 100644 --- a/gitlab/v4/objects/runners.py +++ b/gitlab/v4/objects/runners.py @@ -81,7 +81,7 @@ def all(self, scope: Optional[str] = None, **kwargs: Any) -> List[Runner]: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is + iterator: If set to True and no pagination option is defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) diff --git a/gitlab/v4/objects/users.py b/gitlab/v4/objects/users.py index 09964b1a4..39c243a9f 100644 --- a/gitlab/v4/objects/users.py +++ b/gitlab/v4/objects/users.py @@ -542,12 +542,12 @@ def list(self, **kwargs: Any) -> Union[RESTObjectList, List[RESTObject]]: all: If True, return all the items, without pagination per_page: Number of items to retrieve per request page: ID of the page to return (starts with page 1) - as_list: If set to False and no pagination option is + iterator: If set to True and no pagination option is defined, return a generator instead of a list **kwargs: Extra options to send to the server (e.g. sudo) Returns: - The list of objects, or a generator if `as_list` is False + The list of objects, or a generator if `iterator` is True Raises: GitlabAuthenticationError: If authentication is not correct diff --git a/tests/functional/api/test_gitlab.py b/tests/functional/api/test_gitlab.py index 4684e433b..c9a24a0bb 100644 --- a/tests/functional/api/test_gitlab.py +++ b/tests/functional/api/test_gitlab.py @@ -220,9 +220,20 @@ def test_list_all_true_nowarning(gl): assert len(items) > 20 -def test_list_as_list_false_nowarning(gl): - """Using `as_list=False` will disable the warning""" +def test_list_iterator_true_nowarning(gl): + """Using `iterator=True` will disable the warning""" with warnings.catch_warnings(record=True) as caught_warnings: - items = gl.gitlabciymls.list(as_list=False) + items = gl.gitlabciymls.list(iterator=True) assert len(caught_warnings) == 0 assert len(list(items)) > 20 + + +def test_list_as_list_false_warnings(gl): + """Using `as_list=False` will disable the UserWarning but cause a + DeprecationWarning""" + with warnings.catch_warnings(record=True) as caught_warnings: + items = gl.gitlabciymls.list(as_list=False) + assert len(caught_warnings) == 1 + for warning in caught_warnings: + assert isinstance(warning.message, DeprecationWarning) + assert len(list(items)) > 20 diff --git a/tests/functional/api/test_projects.py b/tests/functional/api/test_projects.py index 50cc55422..8d367de44 100644 --- a/tests/functional/api/test_projects.py +++ b/tests/functional/api/test_projects.py @@ -15,7 +15,7 @@ def test_create_project(gl, user): sudo_project = gl.projects.create({"name": "sudo_project"}, sudo=user.id) created = gl.projects.list() - created_gen = gl.projects.list(as_list=False) + created_gen = gl.projects.list(iterator=True) owned = gl.projects.list(owned=True) assert admin_project in created and sudo_project in created diff --git a/tests/unit/mixins/test_mixin_methods.py b/tests/unit/mixins/test_mixin_methods.py index 06cc3223b..241cba325 100644 --- a/tests/unit/mixins/test_mixin_methods.py +++ b/tests/unit/mixins/test_mixin_methods.py @@ -107,7 +107,7 @@ class M(ListMixin, FakeManager): # test RESTObjectList mgr = M(gl) - obj_list = mgr.list(as_list=False) + obj_list = mgr.list(iterator=True) assert isinstance(obj_list, base.RESTObjectList) for obj in obj_list: assert isinstance(obj, FakeObject) @@ -138,7 +138,7 @@ class M(ListMixin, FakeManager): ) mgr = M(gl) - obj_list = mgr.list(path="/others", as_list=False) + obj_list = mgr.list(path="/others", iterator=True) assert isinstance(obj_list, base.RESTObjectList) obj = obj_list.next() assert obj.id == 42 diff --git a/tests/unit/test_gitlab.py b/tests/unit/test_gitlab.py index 38266273e..44abfc182 100644 --- a/tests/unit/test_gitlab.py +++ b/tests/unit/test_gitlab.py @@ -87,7 +87,7 @@ def resp_page_2(): @responses.activate def test_gitlab_build_list(gl, resp_page_1, resp_page_2): responses.add(**resp_page_1) - obj = gl.http_list("/tests", as_list=False) + obj = gl.http_list("/tests", iterator=True) assert len(obj) == 2 assert obj._next_url == "http://localhost/api/v4/tests?per_page=1&page=2" assert obj.current_page == 1 @@ -122,7 +122,7 @@ def test_gitlab_build_list_missing_headers(gl, resp_page_1, resp_page_2): stripped_page_2 = _strip_pagination_headers(resp_page_2) responses.add(**stripped_page_1) - obj = gl.http_list("/tests", as_list=False) + obj = gl.http_list("/tests", iterator=True) assert len(obj) == 0 # Lazy generator has no knowledge of total items assert obj.total_pages is None assert obj.total is None @@ -133,10 +133,10 @@ def test_gitlab_build_list_missing_headers(gl, resp_page_1, resp_page_2): @responses.activate -def test_gitlab_all_omitted_when_as_list(gl, resp_page_1, resp_page_2): +def test_gitlab_all_omitted_when_iterator(gl, resp_page_1, resp_page_2): responses.add(**resp_page_1) responses.add(**resp_page_2) - result = gl.http_list("/tests", as_list=False, all=True) + result = gl.http_list("/tests", iterator=True, all=True) assert isinstance(result, gitlab.GitlabList) diff --git a/tests/unit/test_gitlab_http_methods.py b/tests/unit/test_gitlab_http_methods.py index 0f0d5d3f9..f3e298f72 100644 --- a/tests/unit/test_gitlab_http_methods.py +++ b/tests/unit/test_gitlab_http_methods.py @@ -438,12 +438,12 @@ def test_list_request(gl): ) with warnings.catch_warnings(record=True) as caught_warnings: - result = gl.http_list("/projects", as_list=True) + result = gl.http_list("/projects", iterator=False) assert len(caught_warnings) == 0 assert isinstance(result, list) assert len(result) == 1 - result = gl.http_list("/projects", as_list=False) + result = gl.http_list("/projects", iterator=True) assert isinstance(result, GitlabList) assert len(list(result)) == 1 @@ -484,12 +484,30 @@ def test_list_request(gl): } +@responses.activate +def test_as_list_deprecation_warning(gl): + responses.add(**large_list_response) + + with warnings.catch_warnings(record=True) as caught_warnings: + result = gl.http_list("/projects", as_list=False) + assert len(caught_warnings) == 1 + warning = caught_warnings[0] + assert isinstance(warning.message, DeprecationWarning) + message = str(warning.message) + assert "`as_list=False` is deprecated" in message + assert "Use `iterator=True` instead" in message + assert __file__ == warning.filename + assert not isinstance(result, list) + assert len(list(result)) == 20 + assert len(responses.calls) == 1 + + @responses.activate def test_list_request_pagination_warning(gl): responses.add(**large_list_response) with warnings.catch_warnings(record=True) as caught_warnings: - result = gl.http_list("/projects", as_list=True) + result = gl.http_list("/projects", iterator=False) assert len(caught_warnings) == 1 warning = caught_warnings[0] assert isinstance(warning.message, UserWarning) @@ -503,10 +521,10 @@ def test_list_request_pagination_warning(gl): @responses.activate -def test_list_request_as_list_false_nowarning(gl): +def test_list_request_iterator_true_nowarning(gl): responses.add(**large_list_response) with warnings.catch_warnings(record=True) as caught_warnings: - result = gl.http_list("/projects", as_list=False) + result = gl.http_list("/projects", iterator=True) assert len(caught_warnings) == 0 assert isinstance(result, GitlabList) assert len(list(result)) == 20 From df072e130aa145a368bbdd10be98208a25100f89 Mon Sep 17 00:00:00 2001 From: Nejc Habjan Date: Sun, 28 Nov 2021 00:50:49 +0100 Subject: [PATCH 004/935] test(gitlab): increase unit test coverage --- gitlab/client.py | 4 +- gitlab/config.py | 8 +-- tests/functional/cli/test_cli.py | 6 +++ tests/unit/helpers.py | 3 ++ tests/unit/mixins/test_mixin_methods.py | 55 +++++++++++++++++++++ tests/unit/test_base.py | 26 +++++++++- tests/unit/test_config.py | 66 +++++++++++++++++++++++-- tests/unit/test_exceptions.py | 12 +++++ tests/unit/test_gitlab.py | 61 ++++++++++++++++++++++- tests/unit/test_gitlab_http_methods.py | 44 ++++++++--------- tests/unit/test_utils.py | 52 +++++++++++++++++++ tox.ini | 1 + 12 files changed, 304 insertions(+), 34 deletions(-) diff --git a/gitlab/client.py b/gitlab/client.py index 2ac5158f6..bba5c1d24 100644 --- a/gitlab/client.py +++ b/gitlab/client.py @@ -208,7 +208,9 @@ def __setstate__(self, state: Dict[str, Any]) -> None: self.__dict__.update(state) # We only support v4 API at this time if self._api_version not in ("4",): - raise ModuleNotFoundError(name=f"gitlab.v{self._api_version}.objects") + raise ModuleNotFoundError( + name=f"gitlab.v{self._api_version}.objects" + ) # pragma: no cover, dead code currently # NOTE: We must delay import of gitlab.v4.objects until now or # otherwise it will cause circular import errors import gitlab.v4.objects diff --git a/gitlab/config.py b/gitlab/config.py index c85d7e5fa..337a26531 100644 --- a/gitlab/config.py +++ b/gitlab/config.py @@ -154,7 +154,7 @@ def _parse_config(self) -> None: # CA bundle. try: self.ssl_verify = _config.get("global", "ssl_verify") - except Exception: + except Exception: # pragma: no cover pass except Exception: pass @@ -166,7 +166,7 @@ def _parse_config(self) -> None: # CA bundle. try: self.ssl_verify = _config.get(self.gitlab_id, "ssl_verify") - except Exception: + except Exception: # pragma: no cover pass except Exception: pass @@ -197,7 +197,9 @@ def _parse_config(self) -> None: try: self.http_username = _config.get(self.gitlab_id, "http_username") - self.http_password = _config.get(self.gitlab_id, "http_password") + self.http_password = _config.get( + self.gitlab_id, "http_password" + ) # pragma: no cover except Exception: pass diff --git a/tests/functional/cli/test_cli.py b/tests/functional/cli/test_cli.py index a8890661f..0da50e6fe 100644 --- a/tests/functional/cli/test_cli.py +++ b/tests/functional/cli/test_cli.py @@ -27,6 +27,12 @@ def test_version(script_runner): assert ret.stdout.strip() == __version__ +def test_config_error_with_help_prints_help(script_runner): + ret = script_runner.run("gitlab", "-c", "invalid-file", "--help") + assert ret.stdout.startswith("usage:") + assert ret.returncode == 0 + + @pytest.mark.script_launch_mode("inprocess") @responses.activate def test_defaults_to_gitlab_com(script_runner, resp_get_project, monkeypatch): diff --git a/tests/unit/helpers.py b/tests/unit/helpers.py index 33a7c7824..54b2b7440 100644 --- a/tests/unit/helpers.py +++ b/tests/unit/helpers.py @@ -4,6 +4,9 @@ from typing import Optional import requests +import responses + +MATCH_EMPTY_QUERY_PARAMS = [responses.matchers.query_param_matcher({})] # NOTE: The function `httmock_response` and the class `Headers` is taken from diff --git a/tests/unit/mixins/test_mixin_methods.py b/tests/unit/mixins/test_mixin_methods.py index 241cba325..c0b0a580b 100644 --- a/tests/unit/mixins/test_mixin_methods.py +++ b/tests/unit/mixins/test_mixin_methods.py @@ -97,8 +97,17 @@ class M(ListMixin, FakeManager): pass url = "http://localhost/api/v4/tests" + headers = { + "X-Page": "1", + "X-Next-Page": "2", + "X-Per-Page": "1", + "X-Total-Pages": "2", + "X-Total": "2", + "Link": ("