Hi Paul, On Tue, 20 Jan 2015, Paul Wise wrote: > + def get_build_reproducibility_content(self): > + url = 'https://reproducible.debian.net/reproducible.json' > + return get_resource_content(url) > + > + def get_build_reproducibility(self): > + reproducibilities = self.get_build_reproducibility_content() > + reproducibilities = json.loads(reproducibilities) > + reproducibilities = dict([(item['package'], item['status']) for item > in reproducibilities]) > + return reproducibilities > + > + def update_action_item(self, package, status): > + 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, > + severity=ActionItem.SEVERITY_NORMAL) > + > + if not (status and status in self.ITEM_DESCRIPTION and > self.ITEM_DESCRIPTION[status]): > + return
Don't you want to delete the action item here if the new status is None or not known ? In fact you probably want to create it the new action_item only after that test too! Or given that you delete obsolete action items later, you might want to do this check first (or outside of the function). > + url = > "https://reproducible.debian.net/rb-pkg/{pkg}.html".format(pkg=package.name) > + action_item.short_description = > self.ITEM_DESCRIPTION[status].format(url=url) > + action_item.save() > + > + def execute(self): > + reproducibilities = self.get_build_reproducibility() > + if reproducibilities is None: > + return get_build_reproducibility() can never return None with your current implementation. To follow the pattern used in other tasks, it should do so when the underlying URL hasn't changed. Instead of using get_resource_content, you must use the HttpCache() directly to be able to retrieve the result of cache.update(). See for example RetrieveLowThresholdNmuTask._retrieve_emails(). (That said this whole pattern should really be factorized in a new helper function) > + with transaction.atomic(): > + > PackageExtractedInfo.objects.filter(key='reproducibility').delete() > + > + packages = [] > + extracted_info = [] > + > + for name, status in reproducibilities.items(): > + try: > + package = SourcePackageName.objects.get(name=name) > + packages.append(package) > + self.update_action_item(package, status) > + except SourcePackageName.DoesNotExist: > + continue > + > + try: > + reproducibility_info = > package.packageextractedinfo_set.get(key='reproducibility') This will always raise the exception since you dropped them all at the start of the transaction. So you can just keep the content of your except clause AIUI. > + reproducibility_info.value['reproducibility'] = status > + except PackageExtractedInfo.DoesNotExist: > + reproducibility_info = PackageExtractedInfo( > + key='reproducibility', > + package=package, > + value={'reproducibility': status}) > + extracted_info.append(reproducibility_info) > + > + > ActionItem.objects.delete_obsolete_items([self.action_item_type], packages) > + PackageExtractedInfo.objects.bulk_create(extracted_info) Except those little points, it looks mostly good. 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/ -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
