Launchpad itself

Code review comment for lp:~michael.nelson/launchpad/567922-binarypackagebuild-packagebuild-3

  1. 567922-binarypackagebuild-packagebuild-3
  2. Merge into db-devel
Revision history for this message
Michael Nelson (michael.nelson) wrote :

On Thu, Apr 29, 2010 at 3:52 PM, Jelmer Vernooij <email address hidden> wrote:
> Review: Approve code*
> Sorry, still haven't sorted out my mailing list subscriptions. I promise I'll do that for the next one and reply inline...
>
> Making the BuildBase methods static is ugly, though I can understand why you're doing it and it seems like a reasonable thing to do for the moment.

Yep - it's temporary just so there's only one implementation while I
keep refactoring (and tests continue to pass). I've updated the
BRANCH.TODO to reflect that (which will ensure the code doesn't land
until it is fixed).

> Some minor issues:
>
> The various versions of _set_build_farm_job have an extra heading space in their docstring.

Fixed.

>
> The extra_info parameter to IPackageBuild.verify is not documented in its interface.

Fixed.

>
> Also, this code will unfortunately conflict once my simplify-uploadprocess branch lands.

Yeah, that's ok. I'm expecting to be resolving conflicts for the next while :)

Incremental diff attached. And I'd already replaced
flush_database_updates as discussed in a previous rev.

Thanks!

changes_jelmer.diff
1=== modified file 'BRANCH.TODO'
2--- BRANCH.TODO 2010-04-13 19:17:03 +0000
3+++ BRANCH.TODO 2010-04-29 15:10:20 +0000
4@@ -2,3 +2,7 @@
5 # landing. There is a test to ensure it is empty in trunk. If there is
6 # stuff still here when you are ready to land, the items should probably
7 # be converted to bugs so they can be scheduled.
8+
9+The static methods on IBuildFarmJob/IPackageBuild should be reverted
10+to normal methods once all *Build classes have transitioned to the new
11+model and IBuildBase is removed.
12
13=== modified file 'lib/lp/buildmaster/interfaces/packagebuild.py'
14--- lib/lp/buildmaster/interfaces/packagebuild.py 2010-04-29 10:56:03 +0000
15+++ lib/lp/buildmaster/interfaces/packagebuild.py 2010-04-29 15:18:45 +0000
16@@ -130,7 +130,13 @@
17 """
18
19 def notify(extra_info=None):
20- """Notify current build state to related people via email."""
21+ """Notify current build state to related people via email.
22+
23+ :param extra_info: Optional extra information that will be included
24+ in the notification email. If the notification is for a
25+ failed-to-upload error then this must be the content of the
26+ upload log.
27+ """
28
29
30 class IPackageBuildSource(Interface):
31
32=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
33--- lib/lp/code/model/sourcepackagerecipebuild.py 2010-04-28 08:32:10 +0000
34+++ lib/lp/code/model/sourcepackagerecipebuild.py 2010-04-29 15:10:58 +0000
35@@ -223,7 +223,7 @@
36 super(SourcePackageRecipeBuildJob, self).__init__()
37
38 def _set_build_farm_job(self):
39- """ Setup the IBuildFarmJob delegate.
40+ """Setup the IBuildFarmJob delegate.
41
42 We override this to provide a delegate specific to package builds."""
43 self.build_farm_job = PackageBuild(self.build)
44
45=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
46--- lib/lp/soyuz/model/buildpackagejob.py 2010-04-28 08:32:10 +0000
47+++ lib/lp/soyuz/model/buildpackagejob.py 2010-04-29 15:11:32 +0000
48@@ -46,7 +46,7 @@
49 super(BuildPackageJob, self).__init__()
50
51 def _set_build_farm_job(self):
52- """ Setup the IBuildFarmJob delegate.
53+ """Setup the IBuildFarmJob delegate.
54
55 We override this to provide a delegate specific to package builds."""
56 self.build_farm_job = PackageBuild(self.build)
57
58=== modified file 'lib/lp/translations/model/translationtemplatesbuildjob.py'
59--- lib/lp/translations/model/translationtemplatesbuildjob.py 2010-04-28 10:59:12 +0000
60+++ lib/lp/translations/model/translationtemplatesbuildjob.py 2010-04-29 15:11:07 +0000
61@@ -49,7 +49,7 @@
62 super(TranslationTemplatesBuildJob, self).__init__(branch_job)
63
64 def _set_build_farm_job(self):
65- """ Setup the IBuildFarmJob delegate.
66+ """Setup the IBuildFarmJob delegate.
67
68 We override this to provide a non-database delegate that simply
69 provides required functionality to the queue system."""

« Back to merge proposal