- Notifications
You must be signed in to change notification settings - Fork675
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 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-io commentedFeb 15, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
|
nejch left a comment
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.
| fromgitlab.clientimportGitlab,GitlabList | ||
| fromgitlab.constimport*# noqa | ||
| fromgitlab.exceptionsimport*# noqa | ||
| fromgitlabimportutils# 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 inclient.
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 utilsThe 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
Uh oh!
There was an error while loading.Please reload this page.
JohnVillalovos commentedFeb 16, 2021
I've updated it and hopefully addressed all the feedback. Thank you. |
nejch left a comment
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 line2bb16fa#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 thenewly created gitlab/client.py file.Update one test case that was depending on requests being defined ingitlab/__init__.py
JohnVillalovos commentedFeb 18, 2021
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 commitd4e7a03 |
nejch commentedFeb 20, 2021
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