-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Return added team membership instance #2956
base: main
Are you sure you want to change the base?
Conversation
bebcdb4
to
f5d5190
Compare
… value, but we didn't honor it so far
f5d5190
to
20d17da
Compare
"DELETE", | ||
self.url, | ||
) | ||
return status == 204 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad practise: #2760
""" | ||
:calls: `DELETE /teams/{team_id}/memberships/{username} <https://docs.github.com/en/rest/reference/teams#remove-team-membership-for-a-user>`_ | ||
""" | ||
assert isinstance(member, github.NamedUser.NamedUser), member | ||
headers, data = self._requester.requestJsonAndCheck("DELETE", f"{self.url}/memberships/{member._identity}") | ||
status, headers, data = self._requester.requestJson("DELETE", f"{self.url}/memberships/{member._identity}") | ||
return status == 204 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -218,6 +219,7 @@ def add_membership(self, member: NamedUser, role: Opt[str] = NotSet) -> None: | |||
headers, data = self._requester.requestJsonAndCheck( | |||
"PUT", f"{self.url}/memberships/{member._identity}", input=put_parameters | |||
) | |||
return github.Membership.Membership(self._requester, headers, data, completed=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to approve this change, but can you extend the tests in tests/Team.py
? Tests already call add_membership
, so simply assert some attributes of the returned Membership
instance.
These are just small improvements.
Specifically:
Membership
instance.bool
to show whether they were succesful.