-
Notifications
You must be signed in to change notification settings - Fork 200
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
Fix root group check to handle standard groups. #634
Fix root group check to handle standard groups. #634
Conversation
Need some input here from others because this code doesn't really make sense to me -- I'm just trying to match the logic that was contained in the original code. It seems to me that only If indeed ONLY @dinesy @gregdenton Any experience in this area? |
cuegui/cuegui/Utils.py
Outdated
@@ -141,14 +141,14 @@ def isRootGroup(object): | |||
"""Returns true if the object is a root, false if not | |||
@return: If the object is a root group | |||
@rtype: bool""" | |||
return object.__class__.__name__ in ["NestedGroup", "Group"] and not object.hasParent() | |||
return object.__class__.__name__ in ["NestedGroup", "Group"] and object.isRootGroup() |
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.
Can these checks use isinstance(object, Group)
instead of the name check?
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.
Done. In one case below I had to use type
to avoid the inheritance.
pycue/opencue/wrappers/group.py
Outdated
""" | ||
return self.data.HasField('parent') | ||
return hasattr(self.data, 'parent') and not self.data.HasField('parent') |
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.
Based on my read of the code, only NestedGroup
can be root group, so I think it's safe to refactor this to be a little more clear. Basically, is this a NestedGroup
and is the parent field empty.
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.
Yeah, that's my read of it too. Ok, I've reorganized the code here and the type checking to match.
PTAL |
@larsbijl @smith1511 @gregdenton Ping -- would like to get this merged ASAP to unbreak tests 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.
LGTM as long as Group.hasParent isn't called anywhere else in the code.
Thanks! It's not -- I added it very recently in #624. |
Didn't catch this during initial testing because unit tests covering this code were added in a separate PR, which I only just merged.
This code was failing for non-
NestedGroup
s because theparent
field doesn't exist at all for those groups. So, expand the check to cover both types of objects.hasParent
is a bit ambiguous because the standardGroup
object does have aparent_id
field, which I thought was confusing. So I renamed that method as well.