Hi Paul, On Sun, 07 Aug 2016, Paul Wise wrote: > I've attached a patch to implement adding multiarch hints to > tracker.d.o. These multiarch hints are aimed at improving Debian's > multiarch support by adding multiarch metadata and fixing file > conflicts. The service providing them is run by Helmut Grohne.
Has this been tested? Because I have reasons to believe that it doesn't work: > +class MultiArchHintsTask(BaseTask): > + ACTIONS_WEB = 'https://wiki.debian.org/MultiArch/Hints' > + ACTIONS_URL = 'https://dedup.debian.net/static/multiarch-hints.yaml' > + ACTION_ITEM_TYPE_NAME = 'debian-multiarch-hints' > + ACTION_ITEM_TEMPLATE = 'debian/multiarch-hints.html' > + ACTION_ITEM_DESCRIPTION = '<a href="{link}">Multiarch hinter</a> reports > {count} issue(s)' > + > + def __init__(self, force_update=False, *args, **kwargs): > + super(MultiArchHintsTask, self).__init__(*args, **kwargs) > + self.force_update = force_update > + self.action_item_type = ActionItemType.objects.create_or_update( > + type_name=self.ACTION_ITEM_TYPE_NAME, > + full_description_template=self.ACTION_ITEM_TEMPLATE) > + self.SEVERITIES = {} > + for value, name in ActionItem.SEVERITIES: > + self.SEVERITIES[name] = value A function transforming severity name into severity value should really be implemented as class method in ActionItem directly (and it should have proper unit tests). > + def get_data(self): > + return get_resource_content(self.ACTIONS_URL) > + > + def get_packages(self): > + data = self.get_data() > + data = yaml.safe_load(data) I would move the yaml parsing in get_data() > + format = data['multiarch-hints-1.0'] That's the part that I would expect to fail with a KeyError. Here you really want to check that data['format'] is equal to 'multiarch-hints-1.0'. > + data = data['hints'] > + packages = {} > + for item in data: > + if 'source' not in item: > + continue > + package = item['source'] > + if package not in packages: > + packages[package] = {} That kind of initialization code can be avoided by defining packages as a collections.defaultdict(dict). > + wishlist = ActionItem.SEVERITY_WISHLIST > + severity = self.SEVERITIES.get(item['severity'], wishlist) > + packages[package]['severity'] = max(severity, > hints[package].get('severity', wishlist)) > + if 'hints' not in packages[package]: > + packages[package]['hints'] = [] > + packages[package]['hints'].append((item['description'], > item['link'])) packages[package].setdefault('hints', []).append(...) > + return packages > + > + def update_action_item(self, package, severity, description, extra_data): > + action_item = > package.get_action_item_for_type(self.action_item_type.type_name) > + if action_item is None: > + action_item = ActionItem( > + package=package, > + item_type=self.action_item_type) > + action_item.severity = severity > + action_item.short_description = description > + action_item.extra_data = extra_data > + action_item.save() > + > + def execute(self): > + packages = self.get_packages() > + if packages is None: > + return I don't think that your code can ever return "None" in get_packages()... > + with transaction.atomic(): > + for name, data in packages.items(): > + try: > + package = SourcePackageName.objects.get(name=name) > + except SourcePackageName.DoesNotExist: > + continue > + > + description = > ACTION_ITEM_DESCRIPTION.format(count=len(data['hints']), > link=self.ACTIONS_WEB) > + self.update_action_item(package, severity, description, > data['hints']) The "severity" variable does not exist here, I assume you wanted to use data['severity'] instead. > + > ActionItem.objects.delete_obsolete_items([self.action_item_type], > packages.keys()) I obviously dislike the lack of unit tests but I would have merged it anyway if it's known to be working. But I don't think that this is the case currently. Cheers, -- Raphaël Hertzog ◈ Debian Developer Support Debian LTS: http://www.freexian.com/services/debian-lts.html Learn to master Debian: http://debian-handbook.info/get/
