Code review comment for lp:~zedtux/bzr-gtk/gravatar-urllib2

> Alright, I've patched my code.
> The only thing I didn't is the AvatarProvider class move into the other file.
> When I've start to copy my code to move it, I was looking at the same time to
> your comments, and I saw that you're requesting me to move this class that
> should be inherited by all other future providers (If one day someone want to
> implement more providers) in the file of one provider, but it make no sense to
> me.
>
> This master class actually only add the common attribute size, but it also
> could implement more attributes/method in the future if needed.
>
> So, we have two possibles:
> - If you don't think this class is necessary, I can delete it.
> - Otherwise, it should stay in its file.
I don't see why it would no longer be necessary if it's in the same file - it defines the interface that providers have to implement, that seems like a perfectly reasonable thing to have, even if it's in the same file.

As part of this, I think it would be useful to define a dummy get_base_url() in AvatarProvider that just raises NotImplementedError.

If you look at other parts of the bzr code you'll see this is a common pattern - a base class that provides a bunch of methods that all raise NotImplementedError and are then overridden by the subclasses. This makes it easy to see what to implement when you create a subclass and it gives a sane error message if a subclass forgot to provide a particular method.

Cheers,

Jelmer