Hello Simon,
On Mon, 18 Aug 2014, Simon Kainz wrote:
> this adds support for data from duck.debian.net.
Thanks for the patch! Here are a few things to fix:
First you should respect PEP8 for the coding style, notably:
- use only spaces to indent, not tabs
- spaces around assignation operatior
You can use "pep8" to check your code (distro-tracker is not fully PEP8
compliant yet, but we try to pay attention for new code).
> >From a0a3810608a306ab77eee214a9be0854e39e3367 Mon Sep 17 00:00:00 2001
> From: Simon Kainz <si...@familiekainz.at>
> Date: Mon, 18 Aug 2014 10:42:28 +0200
> Subject: [PATCH 1/2] add new template for duck extra info.
There's no point in adding the template in a separate commit. Please
squash both commits together.
> --- /dev/null
> +++ b/distro_tracker/vendor/debian/templates/debian/duck-action-item.html
> @@ -0,0 +1,8 @@
> +{% with duck = item.extra_data.duck_link %}
> +{% with issues = item.extra_data.issues_link %}
> +<a href="{{ duck }}">DUCK</a> reports some <a href="{{ issues }}">issues</a>
> concerning upstream URLs defined for this package.
> +{% endwith %}
> +{% endwith %}
The "with" tag doesn't bring anything as you use each variable only once.
Please inline the duck and issues variables.
> From 11e3918f8f9b99b96e0d77c11ddf686dc36d20f1 Mon Sep 17 00:00:00 2001
> From: Simon Kainz <si...@familiekainz.at>
> Date: Mon, 18 Aug 2014 10:43:14 +0200
> Subject: [PATCH 2/2] add data from duck.debian.net into
>
> +class UpdateDebianDuckTask(BaseTask):
> + """
> + A task for updating upstream url issue information on all packages.
> + """
> +
> + DUCK_LINK = 'http://duck.debian.net'
> + # URL of the list of source packages with issues.
> + DUCK_SP_LIST_URL = 'http://duck.debian.net/static/sourcepackages.txt'
> +
> +
Single empty line here.
> + ACTION_ITEM_TYPE_NAME = 'debian-duck'
> + ACTION_ITEM_TEMPLATE = 'debian/duck-action-item.html'
> + ITEM_DESCRIPTION = 'The URL(s) for this package had some recent
> persistent <a href="{issues_link}">issues</a>'
> +
> + def __init__(self, force_update=False, *args, **kwargs):
> + super(UpdateDebianDuckTask, 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)
Please do not use "tabs" but real spaces with 4-space
> + def _get_duck_urls_content(self):
> + """
> + Gets the list of packages with URL issues from
> + duck.debian.net
> +
> + :returns: A array if source package names.
> + """
> +
> + ducklist = get_resource_content(self.DUCK_SP_LIST_URL)
> +
> + packages = []
> + for package_name in ducklist.splitlines():
> + package_name = package_name.strip()
> + packages.append(package_name)
> + return packages
Eek, the mix of spaces and tabs gives a really bad output here when
quoted...
Also get_resource_content will return None when the content of the URL
hasn't changed since last time, you should deal with that case.
> +
> +
A single empty line here.
> + def update_action_item(self,package):
Space after the comma.
> + 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,
> + )
> +
> + issues_link = self.DUCK_LINK + "/static/sp/" + package.name[0] + "/" +
> package.name + ".html"
Are you really using a directory split based on the first character only?
I ask because the debian pool has a special case for "lib*" packages where
it uses the four first letters and many services have been mimicking this
choice.
> + action_item.short_description= self.ITEM_DESCRIPTION.format(issues_link
> = issues_link)
one space around the first "=" (in the assignation) but no spaces
around the second one (named parameter).
> + action_item.save()
> +
> + def execute(self):
> + ducklings= self._get_duck_urls_content()
One space around the equal.
> + if ducklings is None:
> + return
Your current version of _get_duck_urls_content() will never return None
but this is something to fix (cf another comment above).
> + ActionItem.objects.delete_obsolete_items(
> + item_types=[self.action_item_type],
> + non_obsolete_packages=ducklings)
> +
> + packages = SourcePackageName.objects.filter(name__in=ducklings)
> +
> + for package in packages:
> + self.update_action_item(package)
> +
> class UpdateWnppStatsTask(BaseTask):
Please put two empty lines between two class definitions.
It would also be nicer if you could include unit tests for your code. It
should relatively easy to adapt from other similar tasks in
distro_tracker/vendor/debian/tests.py (UpdateDebciStatusTask for example).
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/20140819091305.ga2...@x230-buxy.home.ouaza.com
