Avoid monkey-patching an object from an external module.

[danielballan]

As @ronpandolfi just reminded me, this package monkey-patches an object in databroker

Xi-cam.gui/xicam/gui/bluesky/databroker_catalog_plugin.py

Lines 78 to 81 in f8efbfb

	from databroker import catalog
	catalog.controller = SearchingCatalogController(catalog)
	catalog.view = QListView()
	catalog.name = 'Bluesky Databroker'

This might violate the expectations of another part of the code that is also using that object. In general I think we should try to avoid monkey-patching objects that we import. One possible way to forward is to make plugins via composition instead of inheritance. Xi-cam plugins are expected to have a certain interface, and the names in this interface may sometimes collide with the names of the attributes of the objects they want to "wrap". Composition avoids this problem.

This touches on a larger issue in Xi-cam: the widespread use of singletons enforced at the metaclass (__new__) level. It came up in conversation (but hasn't been recorded to my knowledge) that this makes it difficult to write unit tests.

One alternative is to implement a pattern like this one, which I have seen recur in several libraries, where there is one special instance, but still the ability to make distinct instances in a testing context.

In [12]: from traitlets.config import SingletonConfigurable                                                                                                                                   

In [13]: class A(SingletonConfigurable): 
    ...:     ... 
    ...:                                                                                                                                                                                      

In [14]: A.instance() is A.instance()

In [15]: A() is A()                                                                                                                                                                           
Out[15]: False

I don't know the details of Xi-cam well enough to know whether this pattern is an exact fit, but a classmethod that returns a special instance might be a useful direction to explore.

[ronpandolfi]

In the specific case of the databroker plugin, I plan to fix this by eliminating CatalogPlugin and using direct import. There's no longer a convincing reason to maintain this as a plugin endpoint. #80

We should talk a bit about the second part of your issue. I think the earlier issues with testing plugins might have been related to plugins loading via importlib being different objects from direct imports (@ihumphrey saw this?). I don't think traitlets would solve that problem. Also, I don't think the __new__ singleton pattern is widespread. This was an isolated usage.

Edit: as an interesting note, plugins are actually not required to inherit from their bases now, provided that they meet the expected spec. Not exactly useful, but a curious effect of transitioning to entrypoints.

[danielballan]

That all sounds reasonable to me.

Yeah, I definitely didn’t mean to recommend using traitlets here, just maybe imitating that pattern if it is useful.

I don’t think I’d heard about that specific testing issue. In general when things have global state they are hard to write isolated tests for. If the usage of __new__ is not widespread then maybe this is a nonissue or an easily resolved one.

It is great that it is now possible to write valid plugins without dependency or inheritance. I really like that.