launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/app/templates/launchpad-widget-macros.pt'
> --- lib/lp/app/templates/launchpad-widget-macros.pt	2013-04-22 06:20:18 +0000
> +++ lib/lp/app/templates/launchpad-widget-macros.pt	2015-09-09 14:30:28 +0000
> @@ -29,7 +29,7 @@
>                   tal:define="error widget/error">
>  
>                <label tal:attributes="for widget/name"
> -                     tal:content="widget/label">Label</label>
> +                     tal:content="string:${widget/label}:">Label</label>
>  
>                <span tal:condition="widget/required"
>                      class="fieldRequired"

This change seems wrong, but I also don't see that these macros are even referenced until this branch. Is there another template that replaces it?

> 
> === added file 'lib/lp/code/browser/widgets/gitref.py'
> --- lib/lp/code/browser/widgets/gitref.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/browser/widgets/gitref.py	2015-09-09 14:30:28 +0000
> @@ -0,0 +1,129 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +__all__ = [
> +    'GitRefWidget',
> +    ]
> +
> +from z3c.ptcompat import ViewPageTemplateFile
> +from zope.formlib.interfaces import (
> +    ConversionError,
> +    IInputWidget,
> +    MissingInputError,
> +    WidgetInputError,
> +    )
> +from zope.formlib.utility import setUpWidget
> +from zope.formlib.widget import (
> +    BrowserWidget,
> +    InputErrors,
> +    InputWidget,
> +    )
> +from zope.interface import implementer
> +from zope.schema import (
> +    Choice,
> +    TextLine,
> +    )
> +
> +from lp.app.errors import UnexpectedFormData
> +from lp.app.validators import LaunchpadValidationError
> +from lp.services.webapp.interfaces import (
> +    IAlwaysSubmittedWidget,
> +    IMultiLineWidgetLayout,
> +    )
> +
> +
> +@implementer(IMultiLineWidgetLayout, IAlwaysSubmittedWidget, IInputWidget)
> +class GitRefWidget(BrowserWidget, InputWidget):
> +
> +    template = ViewPageTemplateFile("templates/gitref.pt")
> +    display_label = False
> +    _widgets_set_up = False
> +
> +    def setUpSubWidgets(self):
> +        if self._widgets_set_up:
> +            return
> +        fields = [
> +            Choice(
> +                __name__="repository", title=u"Git repository",
> +                required=False, vocabulary="GitRepository"),
> +            TextLine(
> +                __name__="path", title=u"Git branch path", required=False),

Do we want to drop the " path"? It will usually be a single segment.

> +            ]
> +        for field in fields:
> +            setUpWidget(
> +                self, field.__name__, field, IInputWidget, prefix=self.name)
> +        self._widgets_set_up = True
> +
> +    def setRenderedValue(self, value):
> +        """See `IWidget`."""
> +        self.setUpSubWidgets()
> +        if value is not None:
> +            self.repository_widget.setRenderedValue(value.repository)
> +            self.path_widget.setRenderedValue(value.path)
> +        else:
> +            self.repository_widget.setRenderedValue(None)
> +            self.path_widget.setRenderedValue(None)
> +
> +    def hasInput(self):
> +        """See `IInputWidget`."""
> +        return (
> +            ("%s.repository" % self.name) in self.request.form or
> +            ("%s.path" % self.name) in self.request.form)
> +
> +    def hasValidInput(self):
> +        """See `IInputWidget`."""
> +        try:
> +            self.getInputValue()
> +            return True
> +        except (InputErrors, UnexpectedFormData):
> +            return False
> +
> +    def getInputValue(self):
> +        """See `IInputWidget`."""
> +        self.setUpSubWidgets()
> +        try:
> +            repository = self.repository_widget.getInputValue()
> +        except MissingInputError:
> +            raise WidgetInputError(
> +                self.name, self.label,
> +                LaunchpadValidationError("Please choose a Git repository."))
> +        except ConversionError:
> +            entered_name = self.request.form_ng.getOne(
> +                "%s.repository" % self.name)
> +            raise WidgetInputError(
> +                self.name, self.label,
> +                LaunchpadValidationError(
> +                    "There is no Git repository named '%s' registered in "
> +                    "Launchpad." % entered_name))
> +        if self.path_widget.hasInput():
> +            path = self.path_widget.getInputValue()
> +        else:
> +            path = None
> +        if not path:
> +            raise WidgetInputError(
> +                self.name, self.label,
> +                LaunchpadValidationError("Please enter a Git branch path."))
> +        ref = repository.getRefByPath(path)
> +        if ref is None:
> +            raise WidgetInputError(
> +                self.name, self.label,
> +                LaunchpadValidationError(
> +                    "The repository at %s does not contain a branch named "
> +                    "'%s'." % (repository.display_name, path)))
> +        return ref
> +
> +    def error(self):
> +        """See `IBrowserWidget`."""
> +        try:
> +            if self.hasInput():
> +                self.getInputValue()
> +        except InputErrors as error:
> +            self._error = error
> +        return super(GitRefWidget, self).error()
> +
> +    def __call__(self):
> +        """See `IBrowserWidget`."""
> +        self.setUpSubWidgets()
> +        return self.template()
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2015-08-07 10:09:26 +0000
> +++ lib/lp/snappy/model/snap.py	2015-09-09 14:30:28 +0000
> @@ -123,12 +123,30 @@
>          self.name = name
>          self.description = description
>          self.branch = branch
> -        self.git_repository = git_repository
> -        self.git_path = git_path
> +        if git_ref is not None:
> +            self.git_repository = git_ref.repository
> +            self.git_path = git_ref.path
> +        else:
> +            self.git_repository = None
> +            self.git_path = None

This might as well use the self.git_ref setter.

>          self.require_virtualized = require_virtualized
>          self.date_created = date_created
>          self.date_last_modified = date_created
>  
> +    @property
> +    def git_ref(self):
> +        """See `ISnap`."""
> +        if self.git_repository is not None:
> +            return self.git_repository.getRefByPath(self.git_path)
> +        else:
> +            return None
> +
> +    @git_ref.setter
> +    def git_ref(self, value):
> +        """See `ISnap`."""
> +        self.git_repository = value.repository
> +        self.git_path = value.path

This will crash if you try to set it to None.

> +
>      def _getProcessors(self):
>          return list(Store.of(self).find(
>              Processor,
> 
> === modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
> --- lib/lp/snappy/model/snapbuildbehaviour.py	2015-08-05 12:25:30 +0000
> +++ lib/lp/snappy/model/snapbuildbehaviour.py	2015-09-09 14:30:28 +0000
> @@ -86,9 +86,9 @@
>          args["archive_private"] = build.archive.private
>          if build.snap.branch is not None:
>              args["branch"] = build.snap.branch.bzr_identity
> -        elif build.snap.git_repository is not None:
> +        elif build.snap.git_ref is not None:
>              args["git_repository"] = build.snap.git_repository.git_https_url
> -            args["git_path"] = build.snap.git_path
> +            args["git_path"] = build.snap.git_ref.name

This could use a comment about git clone -b's silliness.

>          else:
>              raise CannotBuild(
>                  "Source branch/repository for ~%s/%s has been deleted." %


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-canonicalise-git-path/+merge/270543
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.