[gpodder-devel] Woodchuck patches: woodchuck integration
- From: thp at gpodder.org (Thomas Perl)
- Date: Wed, 3 Aug 2011 12:00:08 +0200
Hi,
On Tue, Aug 02, 2011 at 04:50:22PM +0200, Neal H. Walfield wrote:
> This patch modifies the gtk frontend to implement the first of these
> two callbacks. It turns out this was surprisingly easy. If this
> approach is acceptable, I'll modify the other frontends similarly.
> [...]
> http://hssl.cs.jhu.edu/~neal/woodchuck/ports/gpodder/commits/36a4a3bf90c3036a99c9bfaa938dd5cd0127024b
Thanks, some feedback:
* The "woodchuck" module should go in src/gpodder/plugins/
* The DEFAULT_PLUGINS in src/gpodder/__init__.py should be modified
accordingly
* What's the purpose of "mainthread.py"? It's not documented :/
* Please remove the shebang line (#!/usr/bin/env python2.5) in
mainthread.py (if you want to run it, you can use "python -m
gpodder.mainthread") - also, don't hardcode python2.5
* Constants (like refresh_interval in woodchuck.py) should be uppercase
* The "config" object is never used in mywoodchuck - is it really
necessary to pass this object through? (if so, why?)
* The extracting of a single podcast channel via URL is done multiple
times in mywoodchuck's code. Better write a get_podcast(url) function
that will take care of the filtering and logging of the warning,
returning None when the podcast channel is not found
* The same is probably true for get_episode(podcast, url), where the
first argument is already a podcast channel object and the second is
the URL of the episode (+returning None, etc)
* For episodes, the unique identifier should be the GUID, because it
might be possible that feeds publish different items at the same URL
(yes, this is very crappy, but it has happened before..)
* The changes support that you implemented in the model is only used
for the code in on_podcast_save? If so, what's the problem with
unconditionally copying the title to the human_readable_name (or
adding a hook for on_podcast_title_changed). The same is for
on_podcast_url_changed - there are only a few cases (HTTP redirect
and after gpodder.net subscription uploads) where the URL of a
podcast really changes in gPodder.
* In the any statements, if you don't use list comprehension, but
rather a generator expression, it might perform better - any can stop
after the first positive result, the list comprehension builds a list
of booleans and then any uses this list to determine its result. Just
leaving out the square brackets should do the trick :)
* Please adhere to PEP-8[1] where possible. Don't add spaces after
function names (NOT: self.auto_download (stream, obj)).
* You can probably shrink the length of "check_subscriptions" by using
set() and its functions difference() and intersection().
* For idle_add, maybe you want to use gpodder.util.idle_add? ;)
HTH :)
Thanks,
Thomas
[1] http://www.python.org/dev/peps/pep-0008/
Other related posts:
