-
Notifications
You must be signed in to change notification settings - Fork 671
Add support for Groups API method transfer. #1807
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1807 +/- ##
==========================================
- Coverage 92.02% 91.93% -0.09%
==========================================
Files 76 76
Lines 4790 4800 +10
==========================================
+ Hits 4408 4413 +5
- Misses 382 387 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Thanks for the PR. Can you please squash the commits down to one commit. And make sure there is a commit message. Also if possible unit and/or functional tests would be great. Thanks again! |
And updating the documentation on how to use it would also be useful. |
Note that the typo fix commit is unrelated to the feature commit. Should I remove it? I looked at the documentation, but it seems to be autogenerated. There are usage examples for a limited number of group methods in |
I wasn't able to find tests for the related group method |
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.
Thanks for this PR @sattlerc! Just another small comment from me.
thought: I also would almost prefer to just call it transfer()
since the transfer direction is inverted from the transfer_project()
in this same class, but unfortunately we have transfer_project()
in the projects module already as well. Maybe that one could be deprecated and have both methods that do "transfer this resource somewhere" called transfer()
. Outside the scope of this PR but open to hear opinions here :)
And thanks for changing the functional code as this does look closer to the rest of the library e.g. further down in this module.
python-gitlab/gitlab/v4/objects/groups.py
Lines 326 to 331 in 9894b35
if parent_id is not None: | |
data["parent_id"] = parent_id | |
return self.gitlab.http_post( | |
"/groups/import", post_data=data, files=files, **kwargs | |
) |
I'll try and add some tests for |
I would prefer |
Yeah, let's do that. This just looks nicer: group = gl.groups.get("gitlab-org/maintainers")
group.transfer("gitlab-com") It also avoids us getting into an awkward situation if GitLab comes up with a "transfer a group into this group" endpoint at some point 😅 (as is the case with |
Okay. I have have also renamed the exception from |
Thanks, LGTM @sattlerc, not sure if John has any more comments. I'll open an accompanying PR for project transfer and you can rebase after merge if you like, I'll ping here. |
Only that tests would be nice. But I think you said you will work on setting up the test framework and then we can land this on top of that. |
Hey @sattlerc I'm really sorry for the mixup here, I prepared the environment in #1833 and was then eager to get this one merged, so I rebased and amended the commit to reflect the new method name. But I pushed to your fork too early and github auto-closed the PR so I can't push/reopen anymore :( We've recreated it here https://github.com/python-gitlab/python-gitlab/pull/1837/files and you remain the author of the feature commit. I hope this is ok with you 🙇 |
Don't worry about it. |
No description provided.