Bug#753733: tracker.debian.org: Learn about auto-removals
Hi Christophe,
On Mon, 18 Aug 2014, Christophe Siraut wrote:
> The attached patch provides a longer explanation.
Much better, thanks!
> > We have more and more tasks that work almost the same. Maybe it's time
> > to have a generic implementation of this typical task? (With a generic way
> > to test all children of the tasks)
>
> We now use the existing get_resource_content function. I am not sure
> about what you mean when you mention: a generic way to test all children
> of the tasks.
I was thinking of having a generic task (i.e. a new class that we can
inherit from in UpdateAutoRemovalsStatsTask) so that we
don't duplicate the logic in __init__(), set_parameters, get_*_stats, etc
and having a set of tests ready to run on any task built on top of the new
generic task.
But it's just a generic remark, it's in no way a pre-condition to get this
patch merged.
On Mon, 18 Aug 2014, Christophe Siraut wrote:
> + def get_autoremovals_stats(self):
> + """
> + Retrieves and parses the autoremoval stats for all packages.
> + Autoremoval stats include the BTS bugs id.
> +
> + :returns: A dict mapping package names to autoremoval stats.
> + """
> + content = get_resource_content(
> + 'http://udd.debian.org/cgi-bin/autoremovals.yaml.cgi')
Please use an https URL.
> + def update_action_item(self, package, stats):
> + """
> + Creates an :class:`ActionItem <distro_tracker.core.models.ActionItem>`
> + instance for the given type indicating that the package has an
> + autoremoval issue.
> + """
> + action_item = package.get_action_item_for_type(self.action_item_type)
> + if not action_item:
> + action_item = ActionItem(
> + package=package,
> + item_type=self.action_item_type)
> +
I would override the severity to ActionItem.SEVERITY_HIGH
> + bugs_dependencies, buggy_dependencies = [], []
> + if 'bugs_dependencies' in stats:
> + bugs_dependencies = stats['bugs_dependencies']
> + if 'buggy_dependencies' in stats:
> + buggy_dependencies = stats['buggy_dependencies']
Please shorten this to something more idiomatic:
bugs_dependencies = stats.get('bugs_dependencies', [])
...
> + all_bugs = stats['bugs'] + bugs_dependencies
> + link = '<a href="http://bugs.debian.org/{}";>{}</a>'
Please use an https:// URL.
> + action_item.short_description = self.ITEM_DESCRIPTION.format(
> + removal_date=stats['removal_date'].strftime('%d %B'),
> + bugs=', '.join(link.format(bug, bug) for bug in all_bugs))
> +
> + action_item.extra_data = {
> + 'stats': stats,
> + 'removal_date': stats['removal_date'].strftime('%a %d %b %Y'),
> + 'bugs': ', '.join(link.format(bug, bug) for bug in stats['bugs']),
> + 'bugs_dependencies': ', '.join(
> + link.format(bug, bug) for bug in bugs_dependencies),
> + 'buggy_dependencies': ' and '.join(
> + ['<a href="/pkg/{}">{}</a>'.format(p, p)
Please use reverse() to generate the /pkg/foo URLs.
> + for p in buggy_dependencies])
> + }
> + action_item.save()
> +
[...]
After that, it should be ready for merge. I don't like very much the lack
of tests but I won't block it on this ground.
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Discover the Debian Administrator's Handbook:
→ http://debian-handbook.info/get/
Reply to: