enthought / envisage

Envisage is a Python-based framework for building applications whose functionalities can be extended by adding "plug-ins".
http://docs.enthought.com/envisage/
Other
81 stars 26 forks source link

PluginManager.include and PluginManager.exclude are not useful #531

Closed mdickinson closed 1 year ago

mdickinson commented 1 year ago

The PluginManager has include and exclude traits, introduced by @mchilvers in commit c503cb53cf48969b9499d64a086ee95470b231f2. However, those traits don't appear to be at all useful in practice. In particular, they have no effect on which plugins are started.

Code ref: https://github.com/enthought/envisage/blob/27014dc983d9e7c10f95d5970a4453981d8417a1/envisage/plugin_manager.py#L60-L71

There are tests here, but they're flawed: the intent is clearly that the include and exclude affect which plugins are started, but what we test is instead what we get from PluginManager.__iter__, which is different.

Given that no-one seems to have noticed that the traits are useless in over 10 years, we should probably just remove them.

mdickinson commented 1 year ago

There are tests here, but they're flawed

To demonstrate: after adding print statements to SimplePlugin.start and SimplePlugin.stop, I get the following output from running test_only_include_plugins_whose_ids_are_in_the_include_list:

(envisage) mdickinson@mirzakhani envisage % python -m unittest envisage.tests.test_plugin_manager.PluginManagerTestCase.test_only_include_plugins_whose_ids_are_in_the_include_list

Starting plugin foo
Starting plugin bar
Starting plugin baz
Stopping plugin baz
Stopping plugin bar
Stopping plugin foo
.
----------------------------------------------------------------------
Ran 1 test in 0.001s

OK

However, from the way that the test is written, the intent seems to be that only foo and bar should be started and stopped.

mdickinson commented 1 year ago

In particular, they have no effect on which plugins are started.

On the other hand, if you have a CompositePluginManager containing a PluginManager that uses include and exclude, then the supposedly "excluded" plugins will be included (i.e., started and stopped) when using the PluginManager, but not when using the CompositePluginManager containing it. I find it hard to believe that this was the intended behaviour.

mdickinson commented 1 year ago

These traits have been deprecated in #544.

I'll open a separate issue targeting 8.0.0 for removal of the traits and associated machinery, and close this one.

mdickinson commented 1 year ago

Opened #547. Closing here.