Merge lp:~intellectronica/launchpad/tags-cloud-improvement into lp:launchpad

In the tags cloud portlet, only display official tags and the ten most popular unofficial ones.

Hi Tom,

This is fine, but it's a bit ugly. It's difficult to understand, yet
it's a short view method. I have some suggestions, but I think it will
always be a slightly ugly method because of the mix of data that is
has to deal with. Oh well.

Approved, but consider my suggestions :)

Thanks, Gavin.

> === modified file 'lib/lp/bugs/browser/bugtarget.py'
> --- lib/lp/bugs/browser/bugtarget.py 2009-09-20 16:05:43 +0000
> +++ lib/lp/bugs/browser/bugtarget.py 2009-09-22 12:49:43 +0000
> @@ -1293,9 +1293,21 @@
> @property
> def tags_cloud_data(self):
> """The data for rendering a tags cloud"""
> - official_tags = set(self.context.official_bug_tags)
> + official_tags = self.context.official_bug_tags
> tags = self.getUsedBugTagsWithURLs()

Okay, this is a bit confusing because tags contains dicts. This could
perhaps be tags_with_urls?

> - tags.sort(key=itemgetter('tag'))
> + popular_tags = [tag['tag'] for tag in sorted(
> + [tag for tag in tags
> + if tag['tag'] not in official_tags],

This could be a generator expression instead of a list
comprehension. Either that, or do a list.sort() as a second step.

> + key=itemgetter('count'))[:10]]
> + tags = [
> + tag for tag in tags
> + if tag['tag'] in official_tags + popular_tags]

Gah, creating temporary lists every time around a loop. I guess there
aren't many items here, but this smells nevertheless.

> + for official_tag in official_tags:
> + if official_tag not in [tag['tag'] for tag in tags]:

This smells again. It's a little bit bad for performance, and a bit
worse for understanding.

> + tags.append({
> + 'tag': official_tag,
> + 'count': 0,
> + 'url': "+bugs?field.tag=%s" % urllib.quote(official_tag)})
> max_count = float(max([1] + [tag['count'] for tag in tags]))
> for tag in tags:
> if tag['tag'] in official_tags:
> @@ -1305,7 +1317,7 @@
> tag['factor'] = 1.5 + (tag['count'] / max_count)
> else:
> tag['factor'] = 1 + (tag['count'] / max_count)
> - return tags
> + return sorted(tags, key=itemgetter('tag'))
>
> @property
> def show_manage_tags_link(self):
>

Thanks for the review. I have now renamed tags to tag_dicts and tag to tag_dict.

Hi Tom,

I agree with Gavin that lines 12-15 should be broken into multiple statements. I read it a few times and before fully understanding the intent.

And at line 20 of your diff you are recalculating the list of tag['tag'] for each official tag. Move that list creation above the for loop.

With those changes you're ok to land on devel.