Code review comment for lp:~jtv/launchpad/translationtemplatesbuild

We did this review from a paste while LP was down. In summary, r=me assuming you'll not land this until you've got a second branch that ensures the builder (current builds/history) pages display these correctly (ie. you'll need to ensure BFJ.getSpecificJob() finds the appropriate adapted.

Thanks!

10:09 < jtv> noodles775: thanks for the review btw… I _would_ point you to my buildfarm work now, but Maintenance. :
/
10:10 * noodles775 breathes a sigh of relief ;)
10:11 < noodles775> jtv: if you can paste your MP (and a diff), I can do that too :)
10:13 < jtv> noodles775: the MP is at https://code.edge.launchpad.net/~jtv/launchpad/translationtemplatesbuild/+merge
/34952
10:13 < noodles775> jtv: sorry, I meant paste the text of your MP to paste.....com :)
10:13 < jtv> noodles775: yes, I'm otp so a little slow sorry :)
10:22 < jtv> noodles775: my diff is here: http://paste.ubuntu.com/490815/
10:31 < noodles775> jtv: Was that a decision to allow both interfaces for TranslationTemplateBuild via ZCML (rather t
han having ITranslationTemplatesBuild inherit from IBuildFarmJob - like IPackageBuild does)?
10:32 < jtv> noodles775: barely registered. :) I tend to avoid interface hierarchies because they can get a little confusing in combination with class hierarchies, but if that's wrong I'll happily change it.
10:32 < jtv> (not that zcml can't get a little confusing…)
10:33 < noodles775> I'm not sure if its better or worse... I think of it with the question: Is the interface ITranslationTemplatesBuild also an IBuildFarmJob? I'm not fussed either, I was just surprised.
10:34 < jtv> We're sort of probing the boundaries between is-a and has-a here, aren't we?
10:35 < jtv> I guess in python we're doing inheritance, just not in the db—in which case perhaps I should change this after all.
11:09 < noodles775> jtv: sheesh, it'll be nice to get rid of the BFJOld stuff (and makeJob). A question, in your makeJob() method, you don't add the TTBJ to the store... am I missing something? I'm wondering if you meant to call TTBJ.create() there?
11:10 < jtv> noodles775: ah good point, thanks
11:11 < jtv> noodles775: ah! Actually I do add it to the store.
11:11 < jtv> noodles775: in terms of db storage, the BranchJob _is_ the TTBJ
11:31 < noodles775> jtv: ah, so TTBJ isn't a storm class itself. Right.
11:31 < jtv> just one more reason to clean out these stables :)
11:41 < noodles775> jtv: so r=me, assuming that you won't be landing this without further branches (ie. to ensure BFJ.getSpecificJob() works for TranslationTemplatesBuild.
11:41 < noodles775> Thanks!
11:42 < jtv> noodles775: that's actually a lot more than I was hoping for, thanks! Any other points I need to cover?
11:43 < noodles775> jtv: er, you could refactor the code and get rid of IBuildFarmJobOld? ;)
11:44 < noodles775> I can't think of any other things... the above will make it self apparent when you look at current builders or builder histories after having dispatched TTBs.
11:45 < noodles775> (sorry, where "the above" is ensuring BFJ.getSpecificJob() works for the translation template builds)
11:45 < jtv> OK, so I'll move on to that next then.