launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === added file 'lib/lp/app/widgets/templates/distributionsourcepackage-picker.pt'
> --- lib/lp/app/widgets/templates/distributionsourcepackage-picker.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/app/widgets/templates/distributionsourcepackage-picker.pt	2016-07-22 16:51:33 +0000
> @@ -0,0 +1,29 @@
> +<tal:root
> +    xmlns:tal="http://xml.zope.org/namespaces/tal";
> +    xmlns:metal="http://xml.zope.org/namespaces/metal";
> +    omit-tag="">
> +
> +<metal:form-picker use-macro="context/@@form-picker-macros/form-picker">
> +  <script metal:fill-slot="add-picker" tal:content="structure string:
> +  LPJS.use('node', 'lp.app.picker', function(Y) {
> +      var config = ${view/json_config};
> +      var distribution_name = '${view/distribution_name}';
> +      var distribution_id = '${view/distribution_id}';
> +      if (distribution_name !== '') {
> +          config.getContextPath = function() {
> +              return '/' + distribution_name;
> +          }
> +      } else if (distribution_id !== '') {
> +          config.getContextPath = function() {
> +              return '/' + Y.DOM.byId(distribution_id).value;
> +          };
> +      }
> +      var show_widget_id = '${view/show_widget_id}';
> +      Y.on('domready', function(e) {
> +          Y.lp.app.picker.addPicker(config, show_widget_id);
> +      });
> +  });
> +  "/>
> +</metal:form-picker>
> +
> +</tal:root>

This whole thing is awfully risky, but no worse than what you copied from.

> 
> === modified file 'lib/lp/registry/vocabularies.py'
> --- lib/lp/registry/vocabularies.py	2016-07-11 21:46:03 +0000
> +++ lib/lp/registry/vocabularies.py	2016-07-22 16:51:33 +0000
> @@ -2100,8 +2100,13 @@
>                  # Search already did the hard work of looking up binary names.
>                  cache = get_property_cache(dsp)
>                  cache.binary_names = binary_names
> -            token = '%s/%s' % (dsp.distribution.name, dsp.name)
> -            return SimpleTerm(dsp, token, token)
> +            # XXX cjwatson 2016-07-22: It's a bit odd for the token to
> +            # return just the source package name and not the distribution
> +            # name as well, but at the moment this is always fed into a
> +            # package name box so things work much better this way.  If we
> +            # ever do a true combined distribution/package picker, then this
> +            # may need to be revisited.
> +            return SimpleTerm(dsp, dsp.name, dsp.name)

Won't this DTWT if parseToken sees a / and overrides the distribution? Perhaps it's worth ensuring in parseToken that the vocab always has a distribution and removing the override support, or is there some callsite that relies on that?

>          raise LookupError(distribution, spn_or_dsp)
>  
>      def getTerm(self, spn_or_dsp):


-- 
https://code.launchpad.net/~cjwatson/launchpad/fix-dsp-vocab-picker/+merge/300782
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.