Launchpad itself

Code review comment for lp://staging/~julian-edwards/launchpad/mechanical-changes-8

mechanical-changes-8
Merge into devel

Revision history for this message

Barry Warsaw (barry) wrote on 2009-09-04:

On Sep 04, 2009, at 01:15 PM, Julian Edwards wrote:

>= Summary =
>3.0 mechanical changes to sourcepackage builds lists

Awesome, thanks for working on these. 3.0 is really shaping up nicely!

>
>== Implementation details ==
>* SourcePackageView no longer inherits from BuildRecords view
>* New view SourcePackageBuildsView for the converted page so that it better
>encapsulates the build-related stuff, and has a separate page title.

I'm finding that refactoring our view classes really helps out. I think the
new header rules will encourage more of that. Ultimately I think it's better
to use mixins and subclasses more and stop overloading our view classes with
tons of functionality.

>* Very trivial template change!
>
>As a bonus, I fixed the headings on the distroseriesbinarypackage-index.pt
>that I committed a couple of hours ago, the h1 and h2 were the wrong way
>around (and nobody spotted it!).

We'll see about this one. :)

>
>== Tests ==
>bin/test -cvvt xx-builds-pages
>
>== Demo and Q/A ==
>For the builds change:
>https://launchpad.dev/ubuntu/warty/+source/mozilla-firefox/+builds
>
>For the h1/h2 swap:
>https://launchpad.dev/ubuntu/warty/+package/mozilla-firefox/

I took a look at this latter page. Once my branch
lp:~barry/launchpad/417089-headings is reviewed and landed, I have some
suggestions for making this page conform to our really-it's-final-this-time
header and title rules. The changes should be simple enough that you can
update the +builds page too, and as I mentioned in IRC, I will give you an
rs=me for a follow up branch to fix this. We may want to give a blanket rs
for any other branches that fix pages to the new rules.

Here's the diff of the changes I had to make.

- @property
- def page_title(self):
- return smartquote(self.context.title)
+ def label(self):
+ return self.context.title

- <h1 tal:content="context/summary"/>
-
   <tal:description replace="structure context/description/fmt:text-to-html">
       Package Description
   </tal:description>

We noticed a few things. First, the def label() should not be necessary.
I'll update my branch so that the base-layout falls back to view.context.title
when view.label is not defined. Second, by default base-layout cgi.escapes
the label, but it should also smartquote it. Third, I think you can basically
get rid of DistroSeriesBinaryPackageView. Once my branch lands, there'll be
no reason not to just use LaunchpadView. Removing code rocks!

rs=me for all those follow up changes (though of course, I'm happy to review
the branch if you want).

In the meantime, I have only a minor suggestion for something to fix as your
branch stands now, so r=me with its consideration. The branch looks great,
thanks.

review approve
status approve

>or
>https://dogfood.launchpad.net/ubuntu/hardy/+package/kmail
>
>= Launchpad lint =
>
>Checking for conflicts. and issues in doctests and templates.
>Running jslint, xmllint, pyflakes, and pylint.
>Using normal rules.
>
>Linting changed files:
> lib/lp/registry/browser/sourcepackage.py
> lib/lp/soyuz/templates/sourcepackage-builds.pt
> lib/lp/soyuz/browser/sourcepackagebuilds.py
> lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt
> lib/lp/soyuz/browser/configure.zcml
>

=== added file 'lib/lp/soyuz/browser/sourcepackagebuilds.py'
--- lib/lp/soyuz/browser/sourcepackagebuilds.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/sourcepackagebuilds.py 2009-09-04 12:22:22 +0000
> @@ -0,0 +1,37 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Browser views for source package builds."""
> +
> +__metaclass__ = type
> +
> +__all__ = [
> + 'SourcePackageBuildsView',
> + ]
> +
> +from lp.soyuz.browser.build import BuildRecordsView
> +from canonical.lazr.utils import smartquote
> +
> +
> +class SourcePackageBuildsView(BuildRecordsView):
> + """A view for (distro series) source package builds."""
> +
> + @property
> + def page_title(self):
> + return smartquote("Builds for " + self.context.title)

This will likely change too once my branch lands, but I didn't try to convert
this view/page/template to the new header stuff. The rs=me above applies to
this as well.

> +
> + @property
> + def search_name(self):
> + """Direct the builds-list template to omit the name search field."""
> + return False
> +
> + @property
> + def default_build_state(self):
> + """Default build state for sourcepackage builds.
> +
> + This overrides the default that is set on BuildRecordsView."""

Close quote on the next line.

> + # None maps to "all states". The reason we display all states on
> + # this page is because it's unlikely that there will be so
> + # many builds that the listing will be overwhelming.
> + return None
> +

=== modified file 'lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt'
--- lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt 2009-09-03 16:38:19 +0000
+++ lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt 2009-09-04 12:43:13 +0000
> @@ -9,12 +9,12 @@
>
> <body>
> <metal:heading fill-slot="heading">
> - <h1 tal:content="view/page_title"/>
> + <h2 tal:content="view/page_title"/>
> </metal:heading>
>
> <div metal:fill-slot="main">
>
> - <h2 tal:content="context/summary"/>
> + <h1 tal:content="context/summary"/>

See above.

>
> <tal:description replace="structure context/description/fmt:text-to-html">
> Package Description

=== modified file 'lib/lp/soyuz/templates/sourcepackage-builds.pt'
--- lib/lp/soyuz/templates/sourcepackage-builds.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/soyuz/templates/sourcepackage-builds.pt 2009-09-04 12:06:54 +0000
> @@ -3,26 +3,15 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
> xmlns:metal="http://xml.zope.org/namespaces/metal"
> xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> - xml:lang="en"
> - lang="en"
> - dir="ltr"
> - metal:use-macro="context/@@main_template/master"
> + metal:use-macro="view/macro:page/main_only"
> i18n:domain="launchpad"
> >
>
> <body>
>
> -<metal:leftportlets fill-slot="portlets_one">
> - <div tal:replace="structure context/@@+portlet-upstream" />
> -</metal:leftportlets>
> -
> -<metal:rightportlets fill-slot="portlets_two">
> - <div tal:replace="structure context/@@+portlet-releases" />
> -</metal:rightportlets>
> -
> <div metal:fill-slot="main">
>
> - <h1><span tal:replace="context/title"/> builds</h1>
> + <h1 tal:content="view/page_title"/>

See above.

>
> <div tal:replace="structure context/@@+builds-list"/>
> </div>

On Sep 04, 2009, at 01:15 PM, Julian Edwards wrote:

>= Summary =
>3.0 mechanical changes to sourcepackage builds lists

Awesome, thanks for working on these.  3.0 is really shaping up nicely!

>
>== Implementation details ==
>* SourcePackageView no longer inherits from BuildRecords view
>* New view SourcePackageBuildsView for the converted page so that it better 
>encapsulates the build-related stuff, and has a separate page title.

I'm finding that refactoring our view classes really helps out.  I think the
new header rules will encourage more of that.  Ultimately I think it's better
to use mixins and subclasses more and stop overloading our view classes with
tons of functionality.

>* Very trivial template change!
>
>As a bonus, I fixed the headings on the distroseriesbinarypackage-index.pt 
>that I committed a couple of hours ago, the h1 and h2 were the wrong way 
>around (and nobody spotted it!).

We'll see about this one. :)

I took a look at this latter page.  Once my branch
lp:~barry/launchpad/417089-headings is reviewed and landed, I have some
suggestions for making this page conform to our really-it's-final-this-time
header and title rules.  The changes should be simple enough that you can
update the +builds page too, and as I mentioned in IRC, I will give you an
rs=me for a follow up branch to fix this.  We may want to give a blanket rs
for any other branches that fix pages to the new rules.

Here's the diff of the changes I had to make.

=== modified file 'lib/lp/soyuz/browser/distroseriesbinarypackage.py'
--- lib/lp/soyuz/browser/distroseriesbinarypackage.py	2009-09-03 16:38:19 +0000
+++ lib/lp/soyuz/browser/distroseriesbinarypackage.py	2009-09-04 15:44:12 +0000
@@ -51,6 +51,5 @@
         self.context = context
         self.request = request
 
-    @property
-    def page_title(self):
-        return smartquote(self.context.title)
+    def label(self):
+        return self.context.title

=== modified file 'lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt'
--- lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt	2009-09-04 12:43:13 +0000
+++ lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt	2009-09-04 15:32:31 +0000
@@ -8,14 +8,8 @@
 >
 
 <body>
-  <metal:heading fill-slot="heading">
-    <h2 tal:content="view/page_title"/>
-  </metal:heading>
-
 <div metal:fill-slot="main">
 
-  <h1 tal:content="context/summary"/>
-
   <tal:description replace="structure context/description/fmt:text-to-html">
       Package Description
   </tal:description>

We noticed a few things.  First, the def label() should not be necessary.
I'll update my branch so that the base-layout falls back to view.context.title
when view.label is not defined.  Second, by default base-layout cgi.escapes
the label, but it should also smartquote it.  Third, I think you can basically
get rid of DistroSeriesBinaryPackageView.  Once my branch lands, there'll be
no reason not to just use LaunchpadView.  Removing code rocks!

rs=me for all those follow up changes (though of course, I'm happy to review
the branch if you want).

In the meantime, I have only a minor suggestion for something to fix as your
branch stands now, so r=me with its consideration.  The branch looks great,
thanks.

review approve
 status approve

>or
>https://dogfood.launchpad.net/ubuntu/hardy/+package/kmail
>
>= Launchpad lint =
>
>Checking for conflicts. and issues in doctests and templates.
>Running jslint, xmllint, pyflakes, and pylint.
>Using normal rules.
>
>Linting changed files:
>  lib/lp/registry/browser/sourcepackage.py
>  lib/lp/soyuz/templates/sourcepackage-builds.pt
>  lib/lp/soyuz/browser/sourcepackagebuilds.py
>  lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt
>  lib/lp/soyuz/browser/configure.zcml
>

=== added file 'lib/lp/soyuz/browser/sourcepackagebuilds.py'
--- lib/lp/soyuz/browser/sourcepackagebuilds.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/sourcepackagebuilds.py	2009-09-04 12:22:22 +0000
> @@ -0,0 +1,37 @@
> +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Browser views for source package builds."""
> +
> +__metaclass__ = type
> +
> +__all__ = [
> +    'SourcePackageBuildsView',
> +    ]
> +
> +from lp.soyuz.browser.build import BuildRecordsView
> +from canonical.lazr.utils import smartquote
> +
> +
> +class SourcePackageBuildsView(BuildRecordsView):
> +    """A view for (distro series) source package builds."""
> +
> +    @property
> +    def page_title(self):
> +        return smartquote("Builds for " + self.context.title)

This will likely change too once my branch lands, but I didn't try to convert
this view/page/template to the new header stuff.  The rs=me above applies to
this as well.

> +
> +    @property
> +    def search_name(self):
> +        """Direct the builds-list template to omit the name search field."""
> +        return False
> +
> +    @property
> +    def default_build_state(self):
> +        """Default build state for sourcepackage builds.
> +
> +        This overrides the default that is set on BuildRecordsView."""

Close quote on the next line.

> +        # None maps to "all states". The reason we display all states on
> +        # this page is because it's unlikely that there will be so
> +        # many builds that the listing will be overwhelming.
> +        return None
> +

=== modified file 'lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt'
--- lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt	2009-09-03 16:38:19 +0000
+++ lib/lp/soyuz/templates/distroseriesbinarypackage-index.pt	2009-09-04 12:43:13 +0000
> @@ -9,12 +9,12 @@
>  
>  <body>
>    <metal:heading fill-slot="heading">
> -    <h1 tal:content="view/page_title"/>
> +    <h2 tal:content="view/page_title"/>
>    </metal:heading>
>  
>  <div metal:fill-slot="main">
>  
> -  <h2 tal:content="context/summary"/>
> +  <h1 tal:content="context/summary"/>

See above.

>  
>    <tal:description replace="structure context/description/fmt:text-to-html">
>        Package Description

=== modified file 'lib/lp/soyuz/templates/sourcepackage-builds.pt'
--- lib/lp/soyuz/templates/sourcepackage-builds.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/soyuz/templates/sourcepackage-builds.pt	2009-09-04 12:06:54 +0000
> @@ -3,26 +3,15 @@
>    xmlns:tal="http://xml.zope.org/namespaces/tal"
>    xmlns:metal="http://xml.zope.org/namespaces/metal"
>    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> -  xml:lang="en"
> -  lang="en"
> -  dir="ltr"
> -  metal:use-macro="context/@@main_template/master"
> +  metal:use-macro="view/macro:page/main_only"
>    i18n:domain="launchpad"
>  >
>  
>  <body>
>  
> -<metal:leftportlets fill-slot="portlets_one">
> -    <div tal:replace="structure context/@@+portlet-upstream" />
> -</metal:leftportlets>
> -
> -<metal:rightportlets fill-slot="portlets_two">
> -    <div tal:replace="structure context/@@+portlet-releases" />
> -</metal:rightportlets>
> -
>  <div metal:fill-slot="main">
>  
> -  <h1><span tal:replace="context/title"/> builds</h1>
> +  <h1 tal:content="view/page_title"/>

See above.

>  
>    <div tal:replace="structure context/@@+builds-list"/>
>  </div>

review: Approve

« Back to merge proposal