Code review comment for lp:~rockstar/launchpad/scanner-events

<salgado> rockstar, ISTM that .run() now expects self.server to be not-None, but do we have anything to make sure contextManager() is called before run(), to make sure self.server is set?
<rockstar> salgado, it's actually cls.server, but yes, the JobCronScript deals with all of that.
<rockstar> salgado, it even uses the context manager stuff that barry posted to the list about.
<salgado> rockstar, would it be worth having an "assert self.server is not None" in run(), just to make it clear?
<rockstar> salgado, well, run is called in a with statement, so it would bomb out before then.
<salgado> also, what do you think of a test showing that all the expected subscribers are actually registered for the event? the existing one only shows (implicitly) that the subscriber for generating diffs is registered, but not the others
<rockstar> AIUI, the with is what handles the contextManager call, and knows how to clean up after itself.
<rockstar> salgado, well, I don't think this fix is the way forward. abentley, thumper, and I are all in agreement there. It's the how we're not quite sure of.
<abentley> salgado: This isn't a nice fix, this is a quick, minimally-invasive fix.
<salgado> ok, maybe that'll be clear for someone reading the whole thing and not just the diff, but maybe a comment explaining how we know self.server will be set before run() is called would be nice
<rockstar> salgado, maybe, but this isn't the only place that uses the contextManager.
<salgado> fair enough, I leave that up to you
<rockstar> If you're implementing JobCronScript, you need to know about contextManager and how it works.
<salgado> but if you're just reading the code, it might take you longer than necessary to understand it