FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(api):add support for creating/editing reviewers in project MRs by spyoungtech · Pull Request #1396 · python-gitlab/python-gitlab · GitHub

  1. python-gitlab
  2. python-gitlab

Repository navigation

feat(api):add support for creating/editing reviewers in project MRs #1396

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
nejch merged 2 commits into from
Jun 26, 2021
Conversation 14 Commits 2 Checks 0 Files changed
Merged

feat(api):add support for creating/editing reviewers in project MRs #1396

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
676d1f6
feat(api): add support for creating/editing reviewers in project merg…
spyoungtech Apr 14, 2021
3d985ee
feat(api): remove responsibility for API inconsistencies for MR revie…
spyoungtech Jun 16, 2021
File filter

Filter by extension

Filter by extension .py  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
2 changes: 2 additions & 0 deletions gitlab/v4/objects/merge_requests.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ class ProjectMergeRequestManager(CRUDMixin, RESTManager):
"remove_source_branch",
"allow_maintainer_to_push",
"squash",
"reviewer_ids",

JohnVillalovos May 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As long as we are updating this I think it would be good to bring it up to date with the current API

https://docs.gitlab.com/ee/api/merge_requests.html#create-mr

These seem missing to me:

  • assignee_ids | integer array | no | The ID of the user(s) to assign the MR to. Set to 0 or provide an empty value to unassign all assignees.
  • reviewer_ids | integer array | no | The ID of the user(s) added as a reviewer to the MR. If set to 0 or left empty, no reviewers are added.
  • remove_source_branch | boolean | no | Flag indicating if a merge request should remove the source branch when merging.

Also it is nice if they are in the order shown in the API docs. As makes it easier to try to figure out what we are missing.

spyoungtech Jun 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for taking a look at this.

Yeah, what's interesting about this particular API is that, unlike most other endpoints, the POST body includes parameters that are not included in the response.

One consequence I noticed is that you'd get a (arguably unexpected) error when trying to access the reviewer_ids attribute because it doesn't really exist in the response.

This is not a huge problem, but was a motivation behind the @property getter/setter for reviewer_ids in an attempt to keep the Python API more consistent.

If the idea is to be able to do something like this:

mr.reviewer_ids = [1,2,3]
mr.save()

Then mr.reviewer_ids should always be an accessible attribute even on a freshly retrieved object, in my opinion. Without the property, this will be an attribute error.

If this is seen as a good approach, I'd be happy to add the same for assignees (it has the same oddities, IIRC).

I'll also be happy to take care of the chore of adding remove_source_branch and double-checking for any other missing parameters for MRs.
),
)
_update_attrs = RequiredOptional(
Expand All @@ -388,6 +389,7 @@ class ProjectMergeRequestManager(CRUDMixin, RESTManager):
"discussion_locked",
"allow_maintainer_to_push",
"squash",
"reviewer_ids",
Comment thread
nejch marked this conversation as resolved.
),
)
_list_filters = (
Expand Down
Back | FazBrowse Home | New Git URL