[gpodder-devel] Woodchuck patches
- From: thp at gpodder.org (Thomas Perl)
- Date: Mon, 1 Aug 2011 11:39:24 +0200
Hi Neal,
On Fri, Jul 29, 2011 at 05:48:03PM +0200, Neal H. Walfield wrote:
> At Thomas' request on IRC: my repository with the Woodchuck changes is
> available at:
>
> http://hssl.cs.jhu.edu/~neal/woodchuck/ports/gpodder.git/
>
> The changes for Woodchuck are on the 'woodchuck' branch. (I haven't
> yet rebased the woodchuck integration patch, but the required
> infrastructure changes are there and tested.)
Here is some feedback on your commits:
* 97165b78eaca440fc17264ddcadd549303f18eba (Add more hooks)
This one looks good to me already; about that "XXX" - yes, you
probably want to call the hook on every downloaded episode (i.e.
get_downloaded_episodes()). After that, it's ready for a merge.
* 2f2c52d13a0a77aec873b5be2717dda6fab2a064 (Interface to register...)
If you enable the HookManager unconditionally, we should probably
remove the "if gpodder.user_hooks is not None" conditions throughout
the code. In this case, we have to make sure that all frontends use
the "Core" class, but that's probably a good idea anyway (especially
since the core class does not depend on many things, and just provides
the DB and configuration)
Some other thing: Why do you need the register_hooks() method?
Wouldn't a hook script in the hooks folder provide the registration
automatically with the current setup?
* 9d8223f626b5f7f3e30151b05304da9badc099e8 (Record the attrs that change)
I don't really like this one, as it involves quite some code each time
an attribute is set. It would be better if you implemented it so that
it only works on save() and there it loads the data from the database
and compares both dictionaries for changes. This way, there's only one
place (the save() method) where the changes are tracked, and it's easy
to maintain and overview this piece of code, while still having the
option to easily disable the change tracking (I don't want to have it
enabled by default, as it does not yet provide an advantage in the
generic case where woodchuck isn't used).
Thanks,
Thomas
Other related posts:
