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/ -- To UNSUBSCRIBE, email to debian-qa-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20140818203204.gb26...@x230-buxy.home.ouaza.com
