Bug#833623: tracker.d.o: add multiarch hints
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/
Reply to: