- Notifications
You must be signed in to change notification settings - Fork676
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
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) | ||
| defrepository_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, | ||
| ) | ||
| returnself.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