-
Notifications
You must be signed in to change notification settings - Fork 671
fix: support int for parent_id
in import_group
#2507
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 #2507 +/- ##
==========================================
+ Coverage 87.59% 96.17% +8.58%
==========================================
Files 87 87
Lines 5665 5676 +11
==========================================
+ Hits 4962 5459 +497
+ Misses 703 217 -486
Flags with carried forward coverage won't be shown. Click here to find out more.
|
15c1b17
to
2d1382f
Compare
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, just a quick initial question on the fix, the rest seems to me an unrelated refactor so I'll look at it a bit later as I'm a bit tired right now 😅
2d1382f
to
b95a6f4
Compare
Sorry for the delay here, I just realized the testing conflicts a bit with those added in https://github.com/python-gitlab/python-gitlab/pull/2488/files#diff-fc2667e8a1b0a974c8447eee8fca931262283245469f31693874cad25651c8f1, I'll get back to this on the weekend :) |
b95a6f4
to
192475a
Compare
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, I had some thoughts on the testing structure but will do a follow-up as we have a big mix now from older tests anyway.
I found the tuple of three values confusing. So instead use a dataclass to return the three values. It is still confusing but a little bit less so. Also add some unit tests
This will also fix other use cases where an integer is passed in to MultipartEncoder. Added unit tests to show it works. Closes: #2506
192475a
to
a41539c
Compare
This will also fix other use cases where an integer is passed in to
MultipartEncoder.
Added unit tests to show it works.
Closes: #2506