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.
Can these checks use isinstance(object, Group) instead of the name check?
There was a problem hiding this comment.
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.
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.
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 |
|
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-
NestedGroups because theparentfield doesn't exist at all for those groups. So, expand the check to cover both types of objects.hasParentis a bit ambiguous because the standardGroupobject does have aparent_idfield, which I thought was confusing. So I renamed that method as well.