search

cytoscape / RCy3

New version of RCy3, redesigned and collaboratively maintained by Cytoscape developer community
MIT License
48 stars 20 forks source link

groupAnnotation cleanup #175

Closed bdemchak closed 2 years ago

bdemchak commented 2 years ago

Is it possible to reach the lappy() function after the paste() function has already reduced the names parameter to a string? If so, can you explain how it's reachable?

This function is documented to no result, but it does return a result (e.g., {'canvas': 'foreground', 'rotation': '0.0', 'name': 'Group 1', 'x': '2449.0', 'y': '1882.0', 'z': '0', 'type': 'org.cytoscape.view.presentation.annotations.GroupAnnotation', 'uuid': '303ac590-495b-44a9-8743-0a8c13e22e6f', 'memberUUIDs': '2ddc0142-042d-453d-b8f6-ac0657f2fa44,9f11373c-9d55-4fc3-bc3d-0ea970a7f041,b24818f8-6878-474f-8fba-5b7489a88d1f'})

Should this function documentation be changed? This would make it more easily possible to nested groups and to delete groups.

Also, if the return value is an empty dictionary, I think it means that there was an error. Can we throw an exception in this case?

yihangx commented 2 years ago

We should return the group uuid.

bdemchak commented 2 years ago

If you return just the UUID instead of the whole dictionary, the caller will have to do a lot of work to figure out what the group contains. Why not return the whole dictionary?

AlexanderPico commented 2 years ago

The caller had to specify which annotations to group, so they already know the contents. I think the only new info is the group UUID. If this is indeed the case, then I'd argue that returning only the new info if better because it's simpler and the caller doesn't have to parse a dictionary to get at the new info.

yihangx commented 2 years ago

Fixed.

bdemchak commented 2 years ago

Should the UUID be returned?

yihangx commented 2 years ago

It returns the UUID now.

AlexanderPico commented 2 years ago

Unused if/else and doc fixes coming in next commit. Thanks for catching these, @bdemchak.