Code review comment for lp:~abentley/launchpad/recipe-index

Thanks for filling me in on all the info Aaron.

On Tue, Mar 23, 2010 at 3:51 PM, Aaron Bentley <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Michael Nelson wrote:
>> Normally we have a human readable displayname, and name is normally
>> cake_recipe, but perhaps you guys decided against this for good
>> reason (we should slugify the displayname for the initial name
>> right?). The mockup for recipe creation assumed it would be a slug:
>> http://people.canonical.com/~michaeln/bfb/create_recipe_v2-collapsed.png
>
> The table does not provide a displayname, so I used the name.  It was an
> error to use "Cake Recipe" as the name.  I should have used
> "cake_recipe" or similar.

OK, so the create validation will need to enforce this I guess. Great.

>> Now I'm sure this *is* something you've discussed - but why isn't the
>> traversal for a recipe (or recipes) off the base branch?
>
> The canonical URL is already defined in stable/dbstable; it was not
> introduced by this branch.  Tim proposed the current URL and no one
> disagreed, so it was documented in
> https://dev.launchpad.net/BuildBranchToArchiveUI/InitialCut
>
> As you note, branch names are long, which is a disadvantage.
> Sourcepackagebranches have even longer names.  It's also trivial to
> change the base branch in a recipe, so varying the name according to the
> base branch would make URLs volatile.

I got to chat briefly with Tim and he had a few other points too, so
just for the record in case anyone else asks the same question (I'll
try to document this on the wiki as a FAQ or something):
{{{
08:43 < noodles775> The only reason I could think of (which Aaron also
highlighted) is that it would be fragile if the base branch changes
etc., but the original mockups didn't allow you to change the base
branch of a recipe.
08:43 < thumper> noodles775: and I picked something
08:44 < noodles775> thumper: no problem, as long as you guys have
thought about it.
08:44 < thumper> noodles775: but what about changing the name of a branch?
08:44 < thumper> noodles775: or changing the owner
08:44 < thumper> noodles775: both of those change the unique name of the branch
08:44 < thumper> noodles775: and since the branch is accessed through
the id, the recipe url would chnage
08:45 < thumper> noodles775: also ~user/+recipe/name is short
08:45 < noodles775> thumper: I see, whereas both of those things
wouldn't change the recipe url where it is currently? Great.
08:45 * thumper nods
}}}

>
>> On a related note: I also agree that we should not need to re-define
>> the buildstate titles/descriptions (what's wrong with "Successfully
>> built 7 minutes ago" in the UI?)
>
> It's not the UI that I was tasked to implement:
> http://people.canonical.com/~rockstar/RecipeView.png

Right, if Paul specifically chose "Successful build" because he
thought "Successfully built" was incorrect, then I think it's worth
updating the BuildStatus as you've done, to improve it.

>
>>. I don't see why we can't define a
>> good set of common titles and descriptions (that's why we moved
>> BuildBase and BuildStatus to lp.buildmaster). The main issue seems to
>> be that the current ones (BuildStatus) are still soyuz-specific and I
>> guess changing them will blow this branch out (due to test failures).
>
> I find the existing ones hard to understand.  That's the biggest issue
> from my point of view.  But they also didn't match the grammar in the UI
> mockup.
>
>> Can you create a bug and XXX here so that we don't forget.
>
> Per Julian, I have changed all the enums to match what Code needs.

I like the changes you've made for chrootwait, superseeded etc., they
all explain the *state* of the build much more simply (ie. "This
build" + BuildStatus.title). It's unfortunate that so many doctests
depend on the titles :/

>
>> Again, something you've probably already discussed, but in the
>> mockups we were planning on presenting the "Packaging branch"
>
> We believe that's not required for the initial cut.  We are going for
> the simplest thing that could possibly work.

Of course. I was just wondering whether it had been considered discussed.

Thanks Aaron.