-
Notifications
You must be signed in to change notification settings - Fork 668
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: remove usage of getattr() #1375
Conversation
JohnVillalovos
commented
Mar 14, 2021
•
edited
Loading
edited
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 the ProjectMergeRequestApprovalManager 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. in test_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 that update_uses_post
exists and is False in test_object_mixin_attributes.py
?