On Friday 28 August 2009 20:36:11 Barry Warsaw wrote:
> Review: Approve code
>
> > == Proposed fix ==
> > Many changes!  3.0 goodness.  View and see.
> > e.g. https://dogfood.launchpad.net/ubuntu/+source/amarok
>
> Looks great, thanks for working on this!

Thanks for your patience with the review Barry, and once again apologies for 
inflicting Soyuz *and* tales on you! :)

> It sounds like you had pretty thorough pre-imps about a number of tricky
> issues.  That helps with the review a lot, thanks for having them.

Yeah, just a few :)

> > I set this up on dogfood since the .dev data is rubbish.
> > See:
> > https://dogfood.launchpad.net/ubuntu/+source/amarok
> > https://dogfood.launchpad.net/ubuntu/+source/firefox-3.5
> > etc.
>
> That was very helpful!

I <3 dogfood.

> The code looks generally pretty good.  I have a number of comments, but I
> think they will be easy to address, so I'll give the branch
> merge-conditional, r=me with their consideration.

Ok great, thanks!  I've addressed all your comments, see inline.

> > +class DistributionSourcePackageActionMenu(
> > +    NavigationMenu, DistributionSourcePackageLinksMixin):
> > +    """Action menu for distro source packages."""
> > +    usedfor = IDistributionSourcePackageActionMenu
> > +    facet = 'overview'
> > +    title = 'Actions'
> > +    links = ('change_log', 'subscribe', 'edit')
>
> While I prefer using tuples for these, as you've shown here, our defacto
> standard is to use lists.  Is there a reason why you did it this way?
> Otherwise, might as well stick with the trend and use a list here too.

I've changed it to lists for now, if we later decide we want tuples as we 
discussed in IRC then it's easily changed.

> >      @property
> >      def page_title(self):
> > @@ -268,6 +312,7 @@
> >          """Render a submit input for the delete_packaging_action."""
> >          assert self.can_delete_packaging, 'User cannot delete
> > Packaging.' return ('<input type="submit" class="button" value="Delete
> > Link" ' +                'style="padding: 0pt; font-size: 80%%" '
>
> Do you think it would be better to move this style setting into a class
> inside style-3-0.css?

I don't think so - this is a one-off style for this particular case (it makes 
the row look stupidly fat otherwise) and it doesn't make sense to me to 
increase the size of the main style sheet for that.

> You should be able to write these last two lines as:
>
>         result = sorted(series, key=operator.attrgetter('version'),
>                         reverse=True)

Heh, yes, thanks for fixing my Python dyslexia.

> > +            if version not in pocket_dict:
> > +                pocket_dict[version] = [pub]
> > +            else:
> > +                pocket_dict[version].append(pub)
>
> What do you think about this instead...
>
>             pocket_dict.setdefault(version, []).append(pub)

I think it's so good, I used it.

> > +            # After the title row, we list each package version that's
> > +            # currently published, and which pockets it's published in.
> > +            pocket_dict = self.published_by_version(package)
> > +            for version in pocket_dict.iterkeys():
>
> Is there a known efficiency reason to use iterkeys()?  Is pocket_dict huge?
> If not, then just use implicit .keys(), e.g.:

Not really - again my Python dyslexia I think!  Thanks :)

> What about splitting this up into multiple lines:
>
>                     now = datetime.now(tz=pytz.UTC)
>                     date_published = most_recent_publication.datepublished
>                     published_since = now - date_published

Yep, done.

> > +                    'pockets': ", ".join(
> > +                        [pub.pocket.name for pub in
> > pocket_dict[version]]),
>
> It might be more readable to move this join() to above the dict
> instantiation.

Ok, done.

> > -    >>> browser.getLink("0.1-1").click()
> > -    >>> browser.url
> > -    'http://localhost/ubuntu/+source/pmount/0.1-1'
> > +    >>> browser.getLink("0.1-2").url
> > +    'http://localhost/ubuntu/+source/pmount/0.1-2'
>
> Better to use print here to reduce noise:
>     >>> print browser.getLink('0.1-2').url
>
>     http://localhost/ubuntu/+source/pmount/0.1-2

Right, I missed that, thanks!

> > +<metal:block fill-slot="head_epilogue">
> > +  <tal:devmode condition="devmode">
> > +    <tal:archive_js define="lp_js string:${icingroot}/build">
> > +      <script type="text/javascript"
> > +              tal:attributes="src string:${lp_js}/soyuz/base.js">
> > +      </script>
> > +    </tal:archive_js>
> > +  </tal:devmode>
> > +</metal:block>
>
> Do you think this is better...
>
> <metal:block fill-slot="head_epilogue">
>   <tal:archive_js define="lp_js string:${icingroot}/build"
>                   tal:condition="devmode">
>      <script type="text/javascript"
>              tal:attributes="src string:${lp_js}/soyuz/base.js" />
>   </tal:archive_js>
> </metal:block>
>
> ?

Partially!  I changed the <script> to use the same-element-closing-style, but 
I didn't remove the outer condition.

When you use a "condition", it doesn't stop the other tal directives in the 
same element from being evaluated, so it would try to define "lp_js" and go 
bang when not in devmode (I think - at least I don't want to change the logic 
here, I copied this block from other working templates).

> Thanks for using the new bugs-stat class!  But ouch! this run-on TAL is
> really hard to read.  Can you reformat it to make the code easier on the
> eyes?

Yes, I fixed that, sorry!

> > +    var base_url = (typeof(LP.client.cache['archive_context_url']) !=
> > "undefined") ? LP.client.cache['archive_context_url'] : '';
>
> Wrap line?

Fixed!

> > +    if (base_url !== ''){
>
> Space between ) and {

Aw, this takes me right back to my C++ days.

Ok, I am doing one more test run and then it's off to PQM.  Thanks a lot for 
reviewing!

J