Skip to content

chore: remove usage of getattr() #1375

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

Merged
merged 1 commit into from
Apr 17, 2021
Merged

chore: remove usage of getattr() #1375

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
3 changes: 2 additions & 1 deletion gitlab/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ class UpdateMixin(_RestManagerBase):
_parent = Optional[base.RESTObject]
_parent_attrs = Dict[str, Any]
_path: Optional[str]
_update_uses_post: bool = False
gitlab: gitlab.Gitlab

def _check_missing_update_attrs(self, data: Dict[str, Any]) -> None:
Expand All @@ -357,7 +358,7 @@ def _get_update_method(
Returns:
object: http_put (default) or http_post
"""
if getattr(self, "_update_uses_post", False):
if self._update_uses_post:
http_method = self.gitlab.http_post
else:
http_method = self.gitlab.http_put
Expand Down
26 changes: 25 additions & 1 deletion gitlab/tests/objects/test_project_merge_request_approvals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
Gitlab API: https://docs.gitlab.com/ee/api/merge_request_approvals.html
"""

import copy

import pytest
import responses
import copy

import gitlab


approval_rule_id = 1
Expand Down Expand Up @@ -230,6 +233,17 @@ def resp_snippet():
yield rsps


def test_project_approval_manager_update_uses_post(project, resp_snippet):
"""Ensure the
gitlab.v4.objects.merge_request_approvals.ProjectApprovalManager object has
_update_uses_post set to True"""
Copy link
Member

Choose a reason for hiding this comment

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

I was originally thinking this would be more appropriate in a separate test/fake manager, to test just this specific aspect of it, e.g. in test_object_mixin_attributes.py. But since it's only used here I think it's ok, maybe for a follow-up PR :)

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 can do that, though I'm not quite sure what you want me to do 😕

Do you want a test that shows that update_uses_post exists and is False in test_object_mixin_attributes.py ?

approvals = project.approvals
assert isinstance(
approvals, gitlab.v4.objects.merge_request_approvals.ProjectApprovalManager
)
assert approvals._update_uses_post == True


def test_list_merge_request_approval_rules(project, resp_snippet):
approval_rules = project.mergerequests.get(1).approval_rules.list()
assert len(approval_rules) == 1
Expand All @@ -239,6 +253,11 @@ def test_list_merge_request_approval_rules(project, resp_snippet):

def test_update_merge_request_approvals_set_approvers(project, resp_snippet):
approvals = project.mergerequests.get(1).approvals
assert isinstance(
approvals,
gitlab.v4.objects.merge_request_approvals.ProjectMergeRequestApprovalManager,
)
assert approvals._update_uses_post == True
response = approvals.set_approvers(
updated_approval_rule_approvals_required,
approver_ids=updated_approval_rule_user_ids,
Expand All @@ -254,6 +273,11 @@ def test_update_merge_request_approvals_set_approvers(project, resp_snippet):

def test_create_merge_request_approvals_set_approvers(project, resp_snippet):
approvals = project.mergerequests.get(1).approvals
assert isinstance(
approvals,
gitlab.v4.objects.merge_request_approvals.ProjectMergeRequestApprovalManager,
)
assert approvals._update_uses_post == True
response = approvals.set_approvers(
new_approval_rule_approvals_required,
approver_ids=new_approval_rule_user_ids,
Expand Down