-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Guilherme Salgado wrote: >> + @property >> + def status(self): >> + """A human-friendly status string.""" >> + description = { >> + BuildStatus.NEEDSBUILD: 'Pending build', >> + BuildStatus.FULLYBUILT: 'Successful build', >> + BuildStatus.FAILEDTOBUILD: 'Failed to build', >> + BuildStatus.MANUALDEPWAIT: >> + 'Could not build because of missing dependencies', >> + BuildStatus.CHROOTWAIT: >> + 'Could not build because of chroot issues', >> + BuildStatus.SUPERSEDED: >> + 'Could not build because source package was superseded', >> + BuildStatus.BUILDING: >> + 'Currently building', >> + BuildStatus.FAILEDTOUPLOAD: >> + 'Could not be uploaded correctly', > > Would it not make sense to have these strings as the title of the items > in the BuildStatus enum? That's Soyuz code, and I don't want to change it unnecessarily. I don't know how the existing titles are used, and whether these strings would make sense in that context. >> === added file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py' >> --- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 1970-01-01 00:00:00 +0000 >> +++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-03-19 14:53:16 +0000 >> +class TestSourcePackageRecipe(TestCaseWithFactory): > > Judging by the test class' name, I was expecting to see a test for > SourcePackageRecipe, but in fact it's testing SPR's default view. How > about renaming the test class to TestSourcePackageRecipeView? Done. > >> + >> + layer = DatabaseFunctionalLayer >> + >> + def makeRecipe(self): >> + chef = self.factory.makePersonNoCommit(displayname='Master Chef', >> + name='chef') >> + chocolate = self.factory.makeProduct(name='chocolate') >> + cake_branch = self.factory.makeProductBranch(owner=chef, name='cake', >> + product=chocolate) > > Mind moving all arguments into the same line, in the calls to > makeProductBranch and makePerson above? They should either be on a line > by themselves or be aligned... Done. >> + def getMainText(self, recipe): >> + browser = self.getUserBrowser(canonical_url(recipe)) >> + return extract_text(find_main_content(browser.contents)) >> + >> + def test_index(self): >> + recipe = self.makeRecipe() >> + build = removeSecurityProxy(self.factory.makeSourcePackageRecipeBuild( >> + recipe=recipe)) >> + build.buildstate = BuildStatus.FULLYBUILT >> + build.datebuilt = datetime(2010, 03, 16, tzinfo=utc) >> + pattern = re.compile(dedent("""\ >> + Master Chef >> + Branches >> + Description >> + This recipe .*changes. >> + Recipe Information >> + Owner: >> + Master Chef >> + Base branch: >> + lp://dev/~chef/chocolate/cake >> + Debian version: >> + 1.0 >> + Distros: >> + Secret Squirrel >> + Build records >> + Successful build.on 2010-03-16 >> + Recipe contents >> + # bzr-builder format 0.2 deb-version 1.0 >> + lp://dev/~chef/chocolate/cake"""), re.S) >> + main_text = self.getMainText(recipe) >> + self.assertTrue(pattern.search(main_text), main_text) >> + >> + def test_index_no_suitable_builders(self): >> + recipe = self.makeRecipe() >> + build = removeSecurityProxy(self.factory.makeSourcePackageRecipeBuild( >> + recipe=recipe)) >> + pattern = re.compile(dedent("""\ >> + Build records >> + No suitable builders >> + Recipe contents"""), re.S) >> + main_text = self.getMainText(recipe) >> + self.assertTrue(pattern.search(main_text), main_text) >> + >> + def makeBuildJob(self, recipe): >> + build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe) >> + buildjob = self.factory.makeSourcePackageRecipeBuildJob( >> + recipe_build=build) >> + return build >> + >> + def test_index_pending(self): >> + recipe = self.makeRecipe() >> + buildjob = self.makeBuildJob(recipe) >> + builder = self.factory.makeBuilder() >> + pattern = re.compile(dedent("""\ >> + Build records >> + Pending build.in .*\(estimated\) >> + Recipe contents"""), re.S) >> + main_text = self.getMainText(recipe) >> + self.assertTrue(pattern.search(main_text), main_text) >> + >> + def test_builds(self): >> + """Ensure SourcePackageRecipe.builds is as described.""" > > s/SourcePackageRecipe/SourcePackageRecipeView/ Done. > >> + recipe = self.makeRecipe() >> + build1 = self.makeBuildJob(recipe=recipe) >> + build2 = self.makeBuildJob(recipe=recipe) >> + build3 = self.makeBuildJob(recipe=recipe) >> + build4 = self.makeBuildJob(recipe=recipe) >> + build5 = self.makeBuildJob(recipe=recipe) >> + build6 = self.makeBuildJob(recipe=recipe) >> + view = SourcePackageRecipeView(recipe, LaunchpadTestRequest) > > You might want to pass None as the request here. If you'd rather pass a > proper request, then you should pass an instance of LTR. None seems to work fine. > >> + self.assertEqual( >> + set([build1, build2, build3, build4, build5, build6]), >> + set(view.builds)) >> + def set_day(build, day): >> + removeSecurityProxy(build).datebuilt = datetime( >> + 2010, 03, day, tzinfo=utc) >> + set_day(build1, 16) >> + set_day(build2, 15) >> + self.assertEqual( >> + set([build1, build3, build4, build5, build6]), >> + set(view.builds)) > > The purpose of this assertion is not clear, I think. Care to add a > comment? Done: # When there are 4+ pending builds, only the the most # recently-completed build is returned (i.e. build1, not build2) >> === modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py' >> --- lib/lp/code/interfaces/sourcepackagerecipe.py 2010-02-05 14:22:03 +0000 >> +++ lib/lp/code/interfaces/sourcepackagerecipe.py 2010-03-19 14:53:16 +0000 >> @@ -42,7 +46,20 @@ >> self.newest_supported = newest_supported >> >> >> -class ISourcePackageRecipe(IHasOwner): >> +class ISourcePackageRecipeData(Interface): >> + """A recipe as database data, not text.""" >> + >> + base_branch = Object( >> + schema=IBranch, title=_("Base branch"), description=_( >> + "The base branch to use when building the recipe.")) >> + >> + deb_version_template = TextLine( >> + title=_('deb-version template'), >> + description = _( >> + 'The template that will be used to generate a deb version.'),) >> + >> + >> +class ISourcePackageRecipe(IHasOwner, ISourcePackageRecipeData): >> """An ISourcePackageRecipe describes how to build a source package. >> >> More precisely, it describes how to combine a number of branches into a >> @@ -87,6 +104,9 @@ >> able to upload to the archive. >> """ >> >> + def getBuilds(pending=False): >> + """Return a ResultSet of all the specified builds.""" > > "... of all the builds in the given state.", maybe? Done. Also documented pending: :param pending: If True, select all builds that are pending. If False, select all builds that are not pending. > >> + >> >> class ISourcePackageRecipeSource(Interface): >> """A utility of this interface can be used to create and access recipes. >> >> === modified file 'lib/lp/code/model/sourcepackagerecipe.py' >> --- lib/lp/code/model/sourcepackagerecipe.py 2010-02-05 15:38:19 +0000 >> +++ lib/lp/code/model/sourcepackagerecipe.py 2010-03-19 14:53:16 +0000 >> @@ -111,3 +118,14 @@ >> self, requester, archive) >> build.queueBuild() >> return build >> + >> + def getBuilds(self, pending=False): > > """See `...`""" Done. >> === modified file 'lib/lp/code/model/sourcepackagerecipedata.py' >> --- lib/lp/code/model/sourcepackagerecipedata.py 2010-02-05 15:07:44 +0000 >> +++ lib/lp/code/model/sourcepackagerecipedata.py 2010-03-19 14:53:16 +0000 >> @@ -9,7 +9,7 @@ >> """ >> >> __metaclass__ = type >> -__all__ = ['_SourcePackageRecipeData'] >> +__all__ = ['SourcePackageRecipeData'] > > Why did you rename it? It has become part of the public API, no longer an implementation detail, because it manages data that is useful to address directly. >> from bzrlib.plugins.builder.recipe import ( >> BaseRecipeBranch, MergeInstruction, NestInstruction, RecipeBranch) >> @@ -50,6 +50,7 @@ >> >> def __init__(self, name, type, comment, line_number, branch, revspec, >> directory, recipe_data, parent_instruction): >> + super( _SourcePackageRecipeDataInstruction, self).__init__() > > Is there a whitespace preceding the class name above? There was. Gone now. >> +
>> +
Distros:
> > Is this really meant to be in the plural? Judging by the code below, > I'd expect to see a single distribution series here. Yes, it is meant to be in the plural. There is a database patch that is just waiting for Bjorn's review to change the relationship to a many-to-many: https://code.edge.launchpad.net/~abentley/launchpad/multiple-series-recipe/+merge/21257 > Also, we shouldn't > be using 'Distros' in the UI, should we? Okay, so what should we be using? >> === modified file 'lib/lp/registry/interfaces/person.py' >> --- lib/lp/registry/interfaces/person.py 2010-03-11 20:59:17 +0000 >> +++ lib/lp/registry/interfaces/person.py 2010-03-19 14:53:16 +0000 >> @@ -874,6 +874,9 @@ >> not teams can be converted into teams. >> """ >> >> + def getRecipe(name): > > I wonder if we shouldn't call this getSourcePackageRecipe(name)? > Although it's a lot longer it doesn't leave one wondering what sort of > recipe could a person have. IMO, SourcePackageRecipe is way too long, and the fewer places we use it, the better. I think the confusion will be short-lived, and less typing is a win. Aaron -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkunedAACgkQ0F+nu1YWqI2+6wCfW9upi9F4kXF+BZnTupByzmXe JawAn3/xTzkesPEGLLJlaKRPhnShmtKP =0bOL -----END PGP SIGNATURE-----