-
Notifications
You must be signed in to change notification settings - Fork 670
refactor: move Gitlab and GitlabList to gitlab/client.py #1301
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 @@
## master #1301 +/- ##
==========================================
- Coverage 80.58% 80.55% -0.03%
==========================================
Files 65 66 +1
Lines 3240 3585 +345
==========================================
+ Hits 2611 2888 +277
- Misses 629 697 +68
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 a lot @JohnVillalovos!
I think you had this mind already if I remember @max-wittig WDYT? It can be split more in a later PR if it balloons.. maybe the http requests stuff could go in its own module at some point.
@@ -32,6 +27,7 @@ | |||
__title__, | |||
__version__, | |||
) | |||
from gitlab.client import Gitlab, GitlabList | |||
from gitlab.const import * # noqa | |||
from gitlab.exceptions import * # noqa | |||
from gitlab import utils # noqa |
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.
Are these 2 imports still needed here (utils and requests_toolbelt)? I think they would only be used internally in client
.
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.
I can remove them
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.
As a note when I removed:
from gitlab import utils
The functional CLI test failed. So I put it back in.
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.
I suspect those might have just been some flaky functional tests, but I agree. we can keep it for now
I've updated it and hopefully addressed all the feedback. Thank you. |
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.
Sorry @JohnVillalovos for the delay, could you just rebase? It's just this one added line 2bb16fa#diff-b0f8dbe3635cab16df9ce4bef4498422dbc0e6ad816820e00637f8f4e4d3bed0R146
Otherwise looks nice to have a clean init, unless @max-wittig has any more comments :)
Move the classes Gitlab and GitlabList from gitlab/__init__.py to the newly created gitlab/client.py file. Update one test case that was depending on requests being defined in gitlab/__init__.py
Done I believe. But please double check. I believe everything in the two classes (Gitlab and GitlabList) have been moved correctly to the new file from commit d4e7a03 |
Thanks, just checked with |
Move the classes Gitlab and GitlabList from gitlab/init.py to the
newly created gitlab/client.py file.
Update one test case that was depending on requests being defined in
gitlab/init.py