- Notifications
You must be signed in to change notification settings - Fork673
chore: remove usage of getattr()#1375
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
chore: remove usage of getattr()#1375
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedMar 14, 2021
Codecov Report
@@ Coverage Diff @@## master #1375 +/- ##======================================= Coverage 79.95% 79.96% ======================================= Files 73 73 Lines 4016 4017 +1 =======================================+ Hits 3211 3212 +1 Misses 805 805
Flags with carried forward coverage won't be shown.Click here to find out more.
Continue to review full report at Codecov.
|
Remove usage of getattr(self, "_update_uses_post", False)Instead add it to class and set default value to False.Add a tests that shows it is set to True for theProjectMergeRequestApprovalManager and ProjectApprovalManager classes.
@nejch If you have time to review, it would be appreciated. |
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 again! :) Left a side note for a follow-up if we get to it.
| def test_project_approval_manager_update_uses_post(project, resp_snippet): | ||
| """Ensure the | ||
| gitlab.v4.objects.merge_request_approvals.ProjectApprovalManager object has | ||
| _update_uses_post set to True""" |
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 was originally thinking this would be more appropriate in a separate test/fake manager, to test just this specific aspect of it, e.g. intest_object_mixin_attributes.py. But since it's only used here I think it's ok, maybe for a follow-up PR :)
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 do that, though I'm not quite sure what you want me to do 😕
Do you want a test that shows thatupdate_uses_post exists and is False intest_object_mixin_attributes.py ?
Uh oh!
There was an error while loading.Please reload this page.