On Mon, 2010-03-22 at 14:09 +0000, Aaron Bentley wrote:
> -----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.
> 

Being able to use the enum's titles both here and in soyuz code will
make for more consistent UI and avoid this extra code, so we all win.
Have you checked what the current title is for them?  Maybe you could
use what's there already?

Julian, what do you think?

> >> === 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.
> 

Ok.

> >>  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.
> 
> >> +            <dl id="distros">
> >> +              <dt>Distros:</dt>
> > 
> > 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
> 

Ok, cool.

> > Also, we shouldn't
> > be using 'Distros' in the UI, should we?
> 
> Okay, so what should we be using?

Distributions, perhaps?  I know we shouldn't use abbreviations in the
UI, but maybe 'Distros' is an exception?  The UI reviewer ought to know.

> 
> >> === 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.

Ok

-- 
Guilherme Salgado <email address hidden>