Skip to content
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

Refactor TreeEditor _new_actions and _menu_new_node to avoid hacky eval #1524

Merged
merged 5 commits into from
Feb 23, 2021

Conversation

aaronayres35
Copy link
Contributor

Part of #1502

This PR supersedes #1518.

This PR follows the approach outlined here: #1518 (comment)

Comment on lines +1322 to +1327
try:
new_object = factory()
except:
from traitsui.api import raise_to_debug
raise_to_debug()
return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try: ... except: ... was necessary since without it the new code changes would cause an exception to be raised if one tried to create a new node for an object with required traits. Previously the hacky eval swallowed this here:

try:
eval(
method_name,
globals(),
{
"object": object,
"editor": self,
"node": node,
"info": info,
"handler": handler,
},
)
except:
from traitsui.api import raise_to_debug
raise_to_debug()
return

Once we complete the second portion of this fix, namely "You then sit down and think about how you need to change the tree node API so that it can better specify non-class factories (ie. an improvement or alternative to get_add and get_name). You basically need some way to specify the name to display in the menu, and the factory function to call for each type of thing you want to add. There may even be something like this already in the API. The default implementation of this should just do what the current code does, but it should allow developers to have their own tree node classes supply alternatives."
we will want to remove this. At that point a programmer will have all the tools needed to specify how to create a new node that has required arguments, and if they do so in a way that we still can not create a node we should complain and raise an exception. However now, with the machinery not yet in place, we don't want this to fail. This preserves the current behavior.

""" Adds a new object to the current node.
"""
node, object, nid = self._data
self._data = None
new_node, new_class = self._node_for_class_name(class_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_node_for_class_name is now no longer called anywhere. Perhaps the method can be deleted? It could be useful in some other case so maybe we want to just leave it, but given it is currently unused, it may make more sense to just delete it and re-add it if a future use case comes about that could make use of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_node_for_class_name looks straightforward so if you don't find any users of it, please go ahead and remove it.

@@ -1072,13 +1072,10 @@ def _new_actions(self, node, object):
name = add_node.get_name(object)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit-picky comment but as long as we are refactoring code in this PR, how about refactoring the _new_actions method to return early if len(add) == 0.

# return early if there are no items to be added in the tree
if len(add) == 0:
    return items

for klass in add:
    ....

Refactoring the code like above makes the code slightly more readable by removing one level of indentation.

Similarly, we can change if add_node is not None to if add_node is None to remove one more level of indentation in the code.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants