feat: add support for merge_base API #2236
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2236 +/- ##
=======================================
Coverage 95.43% 95.44%
=======================================
Files 81 81
Lines 5369 5375 +6
=======================================
+ Hits 5124 5130 +6
Misses 245 245
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JohnVillalovos
left a comment
JohnVillalovos
left a comment
There was a problem hiding this comment.
@nejch Looks good to me. Left a comment, but for sure could be something we don't do or do later.
This is approved, feel free to merge with my blessing!
| @exc.on_http_error(exc.GitlabGetError) | ||
| def repository_merge_base( | ||
| self, refs: List[str], **kwargs: Any | ||
| ) -> Union[Dict[str, Any], requests.Response]: |
There was a problem hiding this comment.
This made me think if we should start forcing these to just be a Dict?
There was a problem hiding this comment.
Thanks @JohnVillalovos! Makes sense to me! I was looking at doing this via overloading http_get/http_request to narrow the types there already, depending on the streamed/raw arguments but it got a bit out of hand. Then I think we wouldn't need to do it in "downstream" methods, IIUC. I will look into it again :)
But it requires quite a few overloads for all the possible signatures. This is what I was looking at https://medium.com/analytics-vidhya/making-sense-of-typing-overload-437e6deecade.
| custom_types={"refs": types.ArrayAttribute}, | ||
| transform_data=True, | ||
| ) | ||
| return self.manager.gitlab.http_get(path, query_data=query_data, **kwargs) |
There was a problem hiding this comment.
To force it to be a Dict I think could do this:
| return self.manager.gitlab.http_get(path, query_data=query_data, **kwargs) | |
| result = self.manager.gitlab.http_get(path, query_data=query_data, raw=False, streamed=False, **kwargs) | |
| if TYPE_CHECKING: | |
| assert isinstance(result, dict) | |
| return result |
Closes #1495
Replaces #1577
Depended on #1699