Code review comment for lp:~barry/bzr/claws

Aaron Bentley (abentley) wrote :

Hash: SHA1

Barry Warsaw wrote:
>> I'm just sayin' _get_compose_commandline should stay true to its roots,
>> and just turn its input into a commandline. This also makes testing the
>> "from" formatting simpler.
>>> I know the code in the branch works for Claws though. What do you think of
>> Aaron's suggestions?
>> Anyhow, "bzr selftest Claws" fails, so this branch Needs Fixing.
> I think I've addressed both of these issues. See the latest incremental diff
> here:
> This plumbs the from_ argument through more of the interfaces, adds
> --mail-from switches to the command line, and adds docstring markup for those
> that were missing. As far as I can tell, selftest passes (not just for
> Claws).
> It also adds some tests for the from_ argument.

This goes a lot farther than I was suggesting. I was trying to describe
a better way of factoring your existing patch, without changing the
big-picture behaviour.

It's good in a way, but it also introduces new problems. If we accept a
"--mail-from" parameter, then we need to notify the user when we can't
honour it, probably by raising an exception.

This means that we need to distinguish between user-supplied --mail-from
and default handling. We could either do default handling in Claws, or
we could provide a flag on MailClient classes indicating whether they
accept a from address, and make the default handling in send conditional
on that flag.

It also seems a bit odd to provide a from_ parameter to Editor that
doesn't do anything, since the Editor client *can* support configuring a
- From address.

If you don't want to do all this, I can understand. Broadening the
scope of the patch also broadens the scope of review. If you'd rather
just get the behaviour for Claws, I think we can get something mergeable
with only small changes to your original patch. I'm happy to discuss it
on IRC.

 review needs-fixing

Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla -


review: Needs Fixing

« Back to merge proposal