Code review comment for lp:~wgrant/launchpad/rescueiflost-and-updatestatus-to-model

Thanks for more cleanups and simplifications William!

It'd be nice if we didn't need to define the functions externally to be able to re-use them, but you clarified why that's necessary. Although I wonder if MockBuilder could just inherit from Builder and stub out the relevant methods as required? (leaving rescuiIfLost/updateStatus)? I'll leave it up to you.

Just a small suggested update for a comment below.

> === modified file 'lib/lp/buildmaster/model/builder.py'
> --- lib/lp/buildmaster/model/builder.py 2010-03-24 12:02:21 +0000
> +++ lib/lp/buildmaster/model/builder.py 2010-03-24 12:02:22 +0000
> @@ -8,6 +8,8 @@
> __all__ = [
> 'Builder',
> 'BuilderSet',
> + 'rescueBuilderIfLost',
> + 'updateBuilderStatus',
> ]
>
> import httplib
> @@ -40,7 +42,8 @@
> from lp.buildmaster.interfaces.buildbase import BuildStatus
> from lp.buildmaster.interfaces.builder import (
> BuildDaemonError, BuildSlaveFailure, CannotBuild, CannotFetchFile,
> - CannotResumeHost, IBuilder, IBuilderSet, ProtocolVersionMismatch)
> + CannotResumeHost, CorruptBuildID, IBuilder, IBuilderSet,
> + ProtocolVersionMismatch)
> from lp.buildmaster.interfaces.buildfarmjobbehavior import (
> BuildBehaviorMismatch)
> from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
> @@ -154,6 +157,71 @@
> raise BuildSlaveFailure(info)
>
>
> +# This is a separate function since MockBuilder needs to use it too.
> +# Do not use it -- (Mock)Builder.rescueIfLost should be used instead.

Perhaps:

These functions are defined here so that both Builder and MockBuilder
can share the implementation for the rescueIfLost/updateStatus methods.

> +def rescueBuilderIfLost(builder, logger=None):
> + """See `IBuilder`."""
> + status_sentence = builder.slaveStatusSentence()

{{{
10:10 < noodles775> wgrant: with your rescueiflost/updatestatus branch, couldn't we do something like rescueIfLost = Builder.rescueIfLost on the MockBuilder
                    if the code is the same? (and not have to define the functions?)
10:10 < wgrant> noodles775: Bound methods check the type of the first argument.
10:10 < wgrant> So no, it's not quite that easy :(
10:10 < wgrant> Er, unbound methods.
10:10 < noodles775> Ok.
10:22 < noodles775> wgrant: in which case, is there an obvious reason that I'm missing for why MockBuilder couldn't inherit from Builder and just overload all its methods as needed?
10:27 < wgrant> noodles775: It looks like a few other mocks do that. So maybe I could.
10:27 < noodles775> wgrant: OK, I'll leave it up to you - I personally think it would improve the code.
10:27 < noodles775> r=me

}}}