Proposal to improve the "setup" phase of notifier plugins

[amotl]

Dear Chris,

thank you again for your responsiveness on the different spots I was touching here. While sweeping through some parts of the code base for the last few days, I think we (re-)discovered a few of the places where you also experienced various stages of pain in the past. I hope to have addressed some of them with my recent patches. If you liked it, I would like to continue just for a few more adjustments, when possible.

In this installment, coming from our valuable discussion at #694 (comment) ff., and after rehashing a quick analysis, I would like to propose two variants, how the plugin setup phase can be improved instead of using code in the module-scope, which is problematic.

With kind regards,
Andreas.

Status quo

I mean i tried mocking the modules like you're doing and the tests failed for me at one point.
I'd also add that i got some REALLY weird errors with the TravisCI Test runner when reloading modules.

I hear you. I've observed similar issues when fiddling around with the aspects of needing to reload modules for the sake of test environment adjustments, while familiarizing myself with the code base and the test suite.

I'm not sure if the errors in your test cases after this tests changes you made now are just cycling through the pains i went through.

Not entirely. After touching that can of worms a few times, I just backed off and followed the way you were doing it. I've never experienced any flukes or weird behavior on this specific topic, other than it is difficult and hairy to handle and get right on its own behalf.

Analysis

I believe we should get rid of the root cause of this issue in the long-term. The root cause is that there is code on the module-level of plugin modules. When aiming to amend the environment of this code for testing purposes, one will invitably have to reload the modules, which is invasive and error-prone.

Proposal

My proposal to get rid of this is to inline the corresponding "setup" code into a generic setup() method on the plugin class itself. In that way, everything can be handled much better than before, both at runtime and at test time.

Another variant of this is to have a plain function on the module scope, like includeme(). That's the way how Pyramid defers component initialization code to program runtime instead of module loading time. It is the includeme() convention and it is a really nice but easy trick which makes the plugin module initialization phase a first class citizen of the whole architecture, instead of having some sloppy if/else/except ImportError jungle on the module scope.

As you can see at the Pyramid code examples, its signature is actually def includeme(config), which means it can accept an eventual application-wide configuration object in order to pre-initialize the component in some way, before it will be instantiated.

When aiming to test such component code, all of this can be well controlled by mocking operations from the test case functions, just by overriding the includeme function, without needing to reload the component module.

I think there will be good times ahead if we can squeeze this in somehow. Let me know what you think about this idea.

[caronc]

It just seems like a lot of effort for little gain. Outside of testing, modules are loaded once at the start and then never again. I guess with the @notify, new stuff can potentially be loaded down the road. But nothing is ever reloaded .

I do like having the modules fail gracefully if dependencies are missing so we can relay that through the internal API to it's users. Not sure how the includeme fits in this paradigm?

[amotl]

I do like having the modules fail gracefully if dependencies are missing so we can relay that through the internal API to it's users. Not sure how the includeme fits in this paradigm?

I would say both of the proposed alternatives fit that purpose even better, because the exceptions can be propagated at runtime, and not only on import time. Imagine the machinery calling notifier.setup(), and the exceptions raised from there can be propagated back to the caller.

[amotl]

But, oh well. I found an immediate drawback on my proposal: If the import statements would be confined within a setup() method or includeme() function, the imported symbols would not be available on the module level, so the main body of the notifier plugin would not be able to access them.

My bad, I apparently did not think this to an end.

It just seems like a lot of effort for little gain.

You are right. Unless I will come up with some better proposal, let's close this again.