- Notifications
You must be signed in to change notification settings - Fork673
feat: add support for merge_base API#2236
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedAug 14, 2022
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.
|
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This made me think if we should start forcing these to just be aDict?
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@JohnVillalovos! Makes sense to me! I was looking at doing this via overloadinghttp_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 athttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To force it to be aDict I think could do this:
| returnself.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) | |
| ifTYPE_CHECKING: | |
| assertisinstance(result,dict) | |
| returnresult |
Closes#1495
Replaces#1577
Depended on#1699