Launchpad itself

Code review comment for lp:~jtv/launchpad/536819-display

536819-display
Merge into db-devel

Revision history for this message

Graham Binns (gmb) wrote on 2010-09-14:

Hi Jeroen,

Nice branch! Since you've updated the UI you should get a UI review for
this from one of our crack UI reviewing team. I've a couple of
suggestions, but they're stylistic only.

> === modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
> --- lib/lp/buildmaster/interfaces/buildfarmjob.py 2010-08-30 15:00:23 +0000
> +++ lib/lp/buildmaster/interfaces/buildfarmjob.py 2010-09-14 11:14:51 +0000
> @@ -256,6 +256,14 @@
>
> was_built = Attribute("Whether or not modified by the builddfarm.")
>
> + # This doesn't belong here. It really belongs in IPackageBuild, but
> + # the TAL assumes it can read this directly.
> + dependencies = exported(
> + TextLine(
> + title=_('Dependencies'), required=False,
> + description=_('Debian-like dependency line that must be satisfied'
> + ' before attempting to build this request.')))
> +

ISTR we're supposed to format these things thus:

    dependencies = exported(
        TextLine(
            title=_('Dependencies'), required=False,
            description=_(
                'Debian-like dependency line that must be satisfied '
                'before attempting to build this request.')))

That seems slightly neater to me, anyway. Might as well update it whilst
you're here.

>
> class ISpecificBuildFarmJob(IBuildFarmJob):
> """A marker interface with which to define adapters for IBuildFarmJob.
>
> === added file 'lib/lp/translations/templates/translationtemplatesbuild-index.pt'
> --- lib/lp/translations/templates/translationtemplatesbuild-index.pt 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/templates/translationtemplatesbuild-index.pt 2010-09-14 11:14:51 +0000
> @@ -0,0 +1,133 @@
> +<html
> + xmlns="http://www.w3.org/1999/xhtml"
> + xmlns:tal="http://xml.zope.org/namespaces/tal"
> + xmlns:metal="http://xml.zope.org/namespaces/metal"
> + xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> + metal:use-macro="view/macro:page/main_only"
> + i18n:domain="launchpad"
> +>
> +
> + <body>
> +
> + <tal:registering metal:fill-slot="registering">
> + created
> + <span tal:content="context/date_created/fmt:displaydate"
> + tal:attributes="title context/date_created/fmt:datetime"
> + >on 2005-01-01</span>
> + </tal:registering>
> +
> + <div metal:fill-slot="main">
> +
> + <div class="yui-g">
> +
> + <div id="status" class="yui-u first">
> + <div class="portlet">

Slightly odd formatting here.

> + <div metal:use-macro="template/macros/status" />
> + </div>
> + </div>
> +
> + <div id="details" class="yui-u">
> + <div class="portlet">

And here.

> + <div metal:use-macro="template/macros/details" />
> + </div>
> + </div>
> +
> + </div> 
> +
> + <div id="buildlog" class="portlet"
> + tal:condition="context/status/enumvalue:BUILDING">
> + <div metal:use-macro="template/macros/buildlog" />
> + </div>
> +
> + </div> 
> +
> +
> +<metal:macros fill-slot="bogus">
> +
> + <metal:macro define-macro="details">
> + <tal:comment replace="nothing">
> + Details section.
> + </tal:comment>
> + <h2>Build details</h2>
> + <p>Branch:
> + <tal:branch replace="structure context/branch/fmt:link">
> + lp:foo/trunk
> + </tal:branch>

This seems odd, too.

> + </p>
> + <tal:targets tal:define="targets view/getTargets">
> + <div tal:condition="targets">
> + For import into:
> + <ul>
> + <li tal:repeat="target targets">
> + <a tal:replace="structure target/fmt:link">gawk trunk series</a>
> + </li>

As does this.

> + </ul>
> + </div>
> + <div tal:condition="not:targets">
> + <em>Not imported anywhere.</em>
> + </div>
> + </tal:targets>
> + </metal:macro>
> +
> + <metal:macro define-macro="status">
> + <tal:comment replace="nothing">
> + Status section.
> + </tal:comment>
> + <h2>Build status</h2>
> + <p>
> + <span tal:replace="structure context/image:icon" />
> + <span tal:attributes="
> + class string:buildstatus${context/status/name};"
> + tal:content="context/status/title">Fully built</span>
> + <tal:building define="builder context/builder"
> + condition="builder">
> + on <a tal:content="builder/title"
> + tal:attributes="href builder/fmt:url"/>
> + </tal:building>
> + </p>
> +
> + <ul>
> + <li tal:define="time view/renderDispatchTime"
> + tal:condition="time"
> + tal:content="structure time">
> + Started 5 minutes ago
> + </li>
> + <li tal:define="time view/renderFinishTime"
> + tal:condition="time"
> + tal:content="structure time">
> + Finished 30 seconds ago
> + <tal:duration define="duration context/duration" condition="duration">
> + (took <span tal:replace="duration/fmt:exactduration" />)
> + </tal:duration>
> + </li>
> +
> + <li tal:define="file context/log"
> + tal:condition="file">
> + <a class="sprite download"
> + tal:attributes="href context/log_url"
> + tal:content="string: buildlog">BUILDLOG</a>
> + (<span tal:replace="file/content/filesize/fmt:bytes" />)
> + </li>
> + </ul>
> + </metal:macro>
> +
> + <metal:macro define-macro="buildlog">
> + <tal:comment replace="nothing">
> + Buildlog section.
> + </tal:comment>
> + <h2>Buildlog</h2>
> + <div id="buildlog-tail" class="logtail"
> + tal:define="logtail context/buildqueue_record/logtail"
> + tal:content="structure logtail/fmt:text-to-html">
> + <p>Things are crashing and burning all over the place.</p>
> + </div>
> + <p class="discreet" tal:condition="view/user">
> + Updated on <span tal:replace="structure view/user/fmt:local-time"/>
> + </p>
> + </metal:macro>
> +
> +</metal:macros>
> +
> +
> + </body>
> +</html>
>

Hi Jeroen,

Nice branch! Since you've updated the UI you should get a UI review for
this from one of our crack UI reviewing team. I've a couple of
suggestions, but they're stylistic only.

> === modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
> --- lib/lp/buildmaster/interfaces/buildfarmjob.py	2010-08-30 15:00:23 +0000
> +++ lib/lp/buildmaster/interfaces/buildfarmjob.py	2010-09-14 11:14:51 +0000
> @@ -256,6 +256,14 @@
>  
>      was_built = Attribute("Whether or not modified by the builddfarm.")
>  
> +    # This doesn't belong here.  It really belongs in IPackageBuild, but
> +    # the TAL assumes it can read this directly.
> +    dependencies = exported(
> +        TextLine(
> +            title=_('Dependencies'), required=False,
> +            description=_('Debian-like dependency line that must be satisfied'
> +                          ' before attempting to build this request.')))
> +

ISTR we're supposed to format these things thus:

That seems slightly neater to me, anyway. Might as well update it whilst
you're here.

>  
>  class ISpecificBuildFarmJob(IBuildFarmJob):
>      """A marker interface with which to define adapters for IBuildFarmJob.
> 
> === added file 'lib/lp/translations/templates/translationtemplatesbuild-index.pt'
> --- lib/lp/translations/templates/translationtemplatesbuild-index.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/templates/translationtemplatesbuild-index.pt	2010-09-14 11:14:51 +0000
> @@ -0,0 +1,133 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml"
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  metal:use-macro="view/macro:page/main_only"
> +  i18n:domain="launchpad"
> +>
> +
> +  <body>
> +
> +    <tal:registering metal:fill-slot="registering">
> +        created
> +        <span tal:content="context/date_created/fmt:displaydate"
> +              tal:attributes="title context/date_created/fmt:datetime"
> +          >on 2005-01-01</span>
> +    </tal:registering>
> +
> +    <div metal:fill-slot="main">
> +
> +      <div class="yui-g">
> +
> +        <div id="status" class="yui-u first">
> +	  <div class="portlet">

Slightly odd formatting here.

> +            <div metal:use-macro="template/macros/status" />
> +          </div>
> +        </div>
> +
> +        <div id="details" class="yui-u">
> +	  <div class="portlet">

And here.

> +            <div metal:use-macro="template/macros/details" />
> +          </div>
> +        </div>
> +
> +      </div> 
> +
> +      <div id="buildlog" class="portlet"
> +           tal:condition="context/status/enumvalue:BUILDING">
> +        <div metal:use-macro="template/macros/buildlog" />
> +      </div>
> +
> +   </div> 
> +
> +
> +<metal:macros fill-slot="bogus">
> +
> +  <metal:macro define-macro="details">
> +    <tal:comment replace="nothing">
> +      Details section.
> +    </tal:comment>
> +    <h2>Build details</h2>
> +    <p>Branch:
> +	<tal:branch replace="structure context/branch/fmt:link">
> +	  lp:foo/trunk
> +	</tal:branch>

This seems odd, too.

> +    </p>
> +    <tal:targets tal:define="targets view/getTargets">
> +      <div tal:condition="targets">
> +        For import into:
> +        <ul>
> +	  <li tal:repeat="target targets">
> +	    <a tal:replace="structure target/fmt:link">gawk trunk series</a>
> +	  </li>

As does this.

> +        </ul>
> +      </div>
> +      <div tal:condition="not:targets">
> +        <em>Not imported anywhere.</em>
> +      </div>
> +    </tal:targets>
> +  </metal:macro>
> +
> +  <metal:macro define-macro="status">
> +    <tal:comment replace="nothing">
> +      Status section.
> +    </tal:comment>
> +    <h2>Build status</h2>
> +    <p>
> +      <span tal:replace="structure context/image:icon" />
> +      <span tal:attributes="
> +            class string:buildstatus${context/status/name};"
> +            tal:content="context/status/title">Fully built</span>
> +      <tal:building define="builder context/builder"
> +                    condition="builder">
> +        on <a tal:content="builder/title"
> +              tal:attributes="href builder/fmt:url"/>
> +      </tal:building>
> +    </p>
> +
> +    <ul>
> +      <li tal:define="time view/renderDispatchTime"
> +          tal:condition="time"
> +          tal:content="structure time">
> +          Started 5 minutes ago
> +      </li>
> +      <li tal:define="time view/renderFinishTime"
> +          tal:condition="time"
> +          tal:content="structure time">
> +        Finished 30 seconds ago
> +        <tal:duration define="duration context/duration" condition="duration">
> +          (took <span tal:replace="duration/fmt:exactduration" />)
> +        </tal:duration>
> +      </li>
> +
> +      <li tal:define="file context/log"
> +          tal:condition="file">
> +        <a class="sprite download"
> +           tal:attributes="href context/log_url"
> +           tal:content="string: buildlog">BUILDLOG</a>
> +        (<span tal:replace="file/content/filesize/fmt:bytes" />)
> +      </li>
> +    </ul>
> +  </metal:macro>
> +
> +  <metal:macro define-macro="buildlog">
> +    <tal:comment replace="nothing">
> +      Buildlog section.
> +    </tal:comment>
> +    <h2>Buildlog</h2>
> +    <div id="buildlog-tail" class="logtail"
> +         tal:define="logtail context/buildqueue_record/logtail"
> +         tal:content="structure logtail/fmt:text-to-html">
> +      <p>Things are crashing and burning all over the place.</p>
> +    </div>
> +    <p class="discreet" tal:condition="view/user">
> +      Updated on <span tal:replace="structure view/user/fmt:local-time"/>
> +    </p>
> +  </metal:macro>
> +
> +</metal:macros>
> +
> +
> +  </body>
> +</html>
>

review: Approve (code)

« Back to merge proposal