Code review comment for lp:~julian-edwards/launchpad/dsp-redesign

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