Code review comment for lp:~jelmer/bzr/config-mail-client

On Mon, Jan 30, 2012 at 07:19:25AM -0000, Vincent Ladeuil wrote:
>
> >>
> >> 2 - an incomplete migration of the mail_client option (which is more complex
> >> too)
> >>
> >> I'm fine landing 1, I think the changes related to 2 should be kept for a
> >> further proposal which can then be completed.
> > I'm not sure I follow in what way (2) is incomplete, can you expand on that ?
> My bad, I read a bit fast and missed:
>
> + mail_client = config_stack.get('mail_client')(config_stack)
>
> \o/
>
> In the old implementation, I disliked the fact that the config object
> was responsible for building (and returning !) a specific object as it
> meant the API became asymmetric: this object couldn't be passed to
> set().
>
> I'm delighted you've addressed that too :)
>
> Unfortunately, while this is precisely where I want us to go, this is
> (also) precisely where tests are failing (bb.test_send).
These should now be fixed - we were missing invalid='error'.

> Moreover, switching to a config_stack as the parameter to build mail
> clients also breaks compatibility. Hence: more complex.
That's why I mentioned it in the API changes section. Do we really
have to worry about external mail client implementations, are there
any out there?

Cheers,

Jelmer