On Sep 01, 2009, at 10:36 AM, Julian Edwards wrote:

>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! :)

Don't worry, I'll get you back someday with Mailman and ZCML. :)

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

Agreed.  I put this in the tech-debt category and we should change it globally
at some point.  I've also put this on the agenda for tomorrow's reviewer
meetings.

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

Hmm.  I wonder if we need a policy for these kinds of things.  OTOH, it /is/
nice to have one-off style changes more local to the code they affect.  I
often find it difficult to figure out what a style affects by just looking at
the css file.

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

Yay!

>> > +            # 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 :)

Rock!

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

Fantastilicious.

>> > +                    '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.

MacGruber.

>> > -    >>> 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!

Sprockets.

>> > +<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).

Sounds good to me.  We are proud Cult of Cargo members.

>> 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!

Vunderbar.

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

Beauty!

>> > +    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!

No problem.  Thanks for making the changes on an already great branch.

-Barry