Skip to content

Remove use of type_manager in apptools.naming, remove type_manager, and remove naming.adapter#219

Merged
aaronayres35 merged 14 commits into
masterfrom
remove-type_manager
Nov 23, 2020
Merged

Remove use of type_manager in apptools.naming, remove type_manager, and remove naming.adapter#219
aaronayres35 merged 14 commits into
masterfrom
remove-type_manager

Conversation

@aaronayres35
Copy link
Copy Markdown
Contributor

@aaronayres35 aaronayres35 commented Nov 17, 2020

fixes #136
part of #146

In this PR, apptools.naming is converted to use traits.adaptation, and the apptools.type_manager subpackage is removed. I did these in the same PR, but if a reviewer would prefer I can undo the last commit and defer the removal of type_manager to another PR.
It is important to note that the code relating to type_manager in apptools.naming was largely untested, and from what I can tell, unused. There is one example using it, but it seems to be broken for other reasons(? I need to investigate).

I also looked for other non-apptools uses of type_manager and could not find any. Also, as the issue suggests I looked into Mayavi to see if it used the type manager's objects directly via the naming module, and it does not appear to do so.

Checklist

  • Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

Copy link
Copy Markdown
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

I'm afraid I am not comfortable with the changes as the adapters logic are not covered in tests.

Comment thread apptools/naming/context_adapter.py Outdated

# The context that the object is in.
context = Instance(Context)
pass
Copy link
Copy Markdown
Contributor

@kitchoi kitchoi Nov 18, 2020

Choose a reason for hiding this comment

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

Where did context = Instance(Context) go?
Most of the subclasses of ContextAdapter are untested. I think this may be breaking InstanceContextAdapter, TraitDictContextAdapter, TraitListContextAdapter, which all reference a context attribute defined on this class, and I don't see context being defined on the Context class.

If we are keeping them, we should be adding tests to ensure this switch to adaptation works.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another possibility is that it has never worked on master, then we can just kill them?

@aaronayres35 aaronayres35 changed the title Replace use of type_manager in apptools.naming with traits.adaptation and remove type_manager Remove use of type_manager in apptools.naming, remove type_manager, and remove naming.adapter Nov 20, 2020
@kitchoi
Copy link
Copy Markdown
Contributor

kitchoi commented Nov 23, 2020

For future reference, these are the observations made that result in the type manager being removed:

The initial observation is that apptools.naming.api.explore is used by mayavi, here: https://github.com/enthought/mayavi/blob/9c128635943e28513776dec68492ffd0268c28d1/mayavi/plugins/mayavi_ui_plugin.py#L207-L213
This makes the explore function available from mayavi2's IPython prompt when the application is run with wxPython backend. Further observations:

However, the code path of apptools.naming.api.explore never requires the type_manager adaptation in apptools.naming.context.

Details of the static code analysis

The adaptation is called here:

def _get_context_adapter(self, obj):

Which is called here:
def _get_next_context(self, name):

Which is, conditionally, called by Context.bind, rebind, unbind, lookup, lookup_binding, list_bindings and list_names.

In the explorer UI, Context.lookup_binding, Context.list_bindings and Context.list_names are called.

For lookup_binding, this lives in a method ensure_visible that is not called anywhere in apptools, and is also not called by the NodeTree base class (the method is not found anywhere in pyface source). So it looks to be dead code.

For list_bindings and list_names, they are always called with an empty string, e.g.:

return node.obj.list_bindings("")

When the argument is an empty string, the Context._get_next_context requiring type managers is not called:

def list_names(self, name=""):
""" Lists the names bound in a context. """
# If the name is empty then the operation takes place in this context.
if len(name) == 0:
names = self._list_names()
# Otherwise, attempt to continue resolution into the next context.
else:
# Parse the name.
components = self._parse_name(name)
if not self._is_bound(components[0]):
raise NameNotFoundError(components[0])
next_context = self._get_next_context(components[0])
names = next_context.list_names("/".join(components[1:]))
return names

So the code path never requires type managers.

Further confirmation with manual testing

(1) explore does not add any adapters prior to instantiating the Explorer:

# Factory function for exploring a Python namespace.
def explore(obj):
""" View a Python object as a naming context. """
root = Binding(name="root", obj=PyContext(namespace=obj))
explorer = Explorer(root=root, size=(1200, 400))
explorer.open()
return

If the adapters are required, the type manager needs to be defined on the context, like in the example:
root = PyFSContext(path=full_path)
root.environment[Context.TYPE_MANAGER] = type_manager

(2) Even if we do define the adapters, they don't make a difference. If we modify the example file to use PyContext instead (like in explore):

    # Create the root context.
    root = PyContext(namespace=person)
    root.environment[Context.TYPE_MANAGER] = type_manager

where person is an HasTraits object with many nested containers looks like this:

class Book(HasTraits):
    author = Str()

class Child(HasTraits):
    name = Str()
    mapping = Dict(Str(), List(Book))

class Person(HasTraits):
    name = Str()
    child = Instance(Child, ())

person = Person(
    name="foo",
    child=Child(name="bar", mapping={"a": [Book(), Book()]}),
)

And if I then applied the fix in enthought/pyface#810 so that it can be launched with wxPython4, we can see something like this (after manually expanding the tree nodes):
Screenshot 2020-11-23 at 12 29 55

If we then remove all the adapters in the example, the view looks exactly the same.

Copy link
Copy Markdown
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM.

We will need a news fragment for this change.

@aaronayres35
Copy link
Copy Markdown
Contributor Author

  • The functionality does work on Python 2 and wxPython 3 (@aaronayres35 please confirm).

Yes, I launched mayavi2 in this environment (which was a python 2.7 environment) and explore was working in the python shell:

(mayavi_demo)~/Desktop/mayavi aayres  (master) $ edm list
# Packages in environment 'mayavi_demo'
#  prefix: '/Users/aayres/.edm/envs/mayavi_demo'
appdirs            1.4.3-1     enthought/free
apptools           4.4.0-10    enthought/free
configobj          5.0.6-2     enthought/free
distribute_remove  1.0.0-4     enthought/free
eam                1.1.0-1     enthought/free
eam_mayavi_app     1.0.2-1     enthought/free
enum34             1.1.6-1     enthought/free
envisage           4.7.0-1     enthought/free
libxml2            2.9.4-1     enthought/free
libxslt            1.1.28-7    enthought/free
mayavi             4.5.0-7     enthought/free
mkl                2018.0.3-1  enthought/free
numpy              1.13.3-4    enthought/free
packaging          16.8-2      enthought/free
pyface             6.0.0-1     enthought/free
pygments           2.2.0-1     enthought/free
pyparsing          2.2.0-1     enthought/free
setuptools         38.2.5-1    enthought/free
six                1.10.0-1    enthought/free
traits             4.6.0-1     enthought/free
traitsui           6.0.0-1     enthought/free
vtk                7.0.0-2     enthought/free
wxpython           3.0.2.0-7   enthought/free

@aaronayres35 aaronayres35 merged commit 4554277 into master Nov 23, 2020
@aaronayres35 aaronayres35 deleted the remove-type_manager branch November 23, 2020 14:08
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.

Remove apptools.type_manager?

2 participants