Merge lp:~jelmer/bzr/config-mail-client into lp:bzr

[Jelmer Vernooij]

Use config stacks for 'mail_client' configuration option.

Convert the ''mail_client'' setting over to using config stacks.

[Vincent Ladeuil (vila)]

This seems to mix to changes:
1 - the one described in the news entry,

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.

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

Hi,

Thanks for all the config reviews!

> This seems to mix to changes:
> 1 - the one described in the news entry,
>
> 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 ?

[Vincent Ladeuil (vila)]

    >> This seems to mix to changes:
    >> 1 - the one described in the news entry,

Meh, typed too fast, I meant 'email' here not 'mail_client'. But even
that is not that easy, see below.

    >>
    >> 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).

Moreover, switching to a config_stack as the parameter to build mail
clients also breaks compatibility. Hence: more complex.

[Jelmer Vernooij (jelmer)]

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

[Vincent Ladeuil (vila)]

> > 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'.

Ha cool, sorry, I didn't dig.

>
> > 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?

Probably not that much and they could be fixed easily. But I'm more worried
about users of get_mail_client (which you've deleted).

Greping through all the plugins I have source for, I found hits for
grep_mail_client in bzr-gtk and... that's all ? Wow, will be easy to fix
then :)

May be deprecating and throwing an error from this method would make it
clearer for potential users ?

You still have a failing (and eager) test (<mutter> split ! </mutter) using
get_mail_client by the way.

ERROR: test_config.TestBranchConfigItems.test_get_mail_client

Other than that, I'm glad the invalid='error' was enough and it now seems
more landable.

Let's share the blame if things go wrong, fix the test and land ;)

review: Needs Fixing

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-01-28 17:58:09 +0000
+++ bzrlib/config.py	2012-02-02 12:15:24 +0000
@@ -92,7 +92,6 @@
     lazy_regex,
     library_state,
     lockdir,
-    mail_client,
     mergetools,
     osutils,
     symbol_versioning,
@@ -240,16 +239,6 @@
         return diff.DiffFromTool.from_string(cmd, old_tree, new_tree,
                                              sys.stdout)
 
-    def get_mail_client(self):
-        """Get a mail client to use"""
-        selected_client = self.get_user_option('mail_client')
-        _registry = mail_client.mail_client_registry
-        try:
-            mail_client_class = _registry.get(selected_client)
-        except KeyError:
-            raise errors.UnknownMailClient(selected_client)
-        return mail_client_class(self)
-
     def _get_signature_checking(self):
         """Template method to override signature checking policy."""
 
@@ -2803,6 +2792,8 @@
 Standard log formats are ``long``, ``short`` and ``line``. Additional formats
 may be provided by plugins.
 '''))
+option_registry.register_lazy('mail_client', 'bzrlib.mail_client',
+    'opt_mail_client')
 option_registry.register(
     Option('output_encoding',
            help= 'Unicode encoding for output'

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2012-01-31 18:25:57 +0000
+++ bzrlib/errors.py	2012-02-02 12:15:24 +0000
@@ -2768,14 +2768,6 @@
     _fmt = "No mail-to address (--mail-to) or output (-o) specified."
 
 
-class UnknownMailClient(BzrError):
-
-    _fmt = "Unknown mail client: %(mail_client)s"
-
-    def __init__(self, mail_client):
-        BzrError.__init__(self, mail_client=mail_client)
-
-
 class MailClientNotFound(BzrError):
 
     _fmt = "Unable to find mail client with the following names:"\

=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py	2011-12-19 10:58:39 +0000
+++ bzrlib/mail_client.py	2012-02-02 12:15:24 +0000
@@ -24,6 +24,7 @@
 
 import bzrlib
 from bzrlib import (
+    config as _mod_config,
     email_message,
     errors,
     msgeditor,
@@ -111,7 +112,7 @@
         if body == '':
             raise errors.NoMessageSupplied()
         email_message.EmailMessage.send(self.config,
-                                        self.config.username(),
+                                        self.config.get('email'),
                                         to,
                                         subject,
                                         body,
@@ -378,7 +379,7 @@
                  extension, body=None, from_=None):
         """See ExternalMailClient._compose"""
         if from_ is None:
-            from_ = self.config.get_user_option('email')
+            from_ = self.config.get('email')
         super(Claws, self)._compose(prompt, to, subject, attach_path,
                                     mime_subtype, extension, body, from_)
 
@@ -617,11 +618,11 @@
         """See MailClient.compose"""
         try:
             return self._mail_client().compose(prompt, to, subject,
-                                               attachment, mimie_subtype,
+                                               attachment, mime_subtype,
                                                extension, basename, body)
         except errors.MailClientNotFound:
             return Editor(self.config).compose(prompt, to, subject,
-                          attachment, mimie_subtype, extension, body)
+                          attachment, mime_subtype, extension, body)
 
     def compose_merge_request(self, to, subject, directive, basename=None,
                               body=None):
@@ -636,4 +637,5 @@
                               help=DefaultMail.__doc__)
 mail_client_registry.default_key = 'default'
 
-
+opt_mail_client = _mod_config.RegistryOption('mail_client',
+        mail_client_registry, help='E-mail client to use.', invalid='error')

=== modified file 'bzrlib/send.py'
--- bzrlib/send.py	2012-01-25 15:53:18 +0000
+++ bzrlib/send.py	2012-02-02 12:15:24 +0000
@@ -49,9 +49,10 @@
     branch.lock_write()
     try:
         if output is None:
+            config_stack = branch.get_config_stack()
             if mail_to is None:
-                mail_to = branch.get_config_stack().get('submit_to')
-            mail_client = branch.get_config().get_mail_client()
+                mail_to = config_stack.get('submit_to')
+            mail_client = config_stack.get('mail_client')(config_stack)
             if (not getattr(mail_client, 'supports_body', False)
                 and body is not None):
                 raise errors.BzrCommandError(gettext(

=== modified file 'bzrlib/tests/blackbox/test_send.py'
--- bzrlib/tests/blackbox/test_send.py	2012-01-25 15:53:18 +0000
+++ bzrlib/tests/blackbox/test_send.py	2012-02-02 12:15:24 +0000
@@ -193,25 +193,26 @@
 
     def test_mailto_option(self):
         b = branch.Branch.open('branch')
-        b.get_config().set_user_option('mail_client', 'editor')
+        b.get_config_stack().set('mail_client', 'editor')
         self.run_bzr_error(
             ('No mail-to address \\(--mail-to\\) or output \\(-o\\) specified',
             ), 'send -f branch')
-        b.get_config().set_user_option('mail_client', 'bogus')
+        b.get_config_stack().set('mail_client', 'bogus')
         self.run_send([])
-        self.run_bzr_error(('Unknown mail client: bogus',),
+        self.run_bzr_error(('Bad value "bogus" for option "mail_client"',),
                            'send -f branch --mail-to [e-mail]')
         b.get_config_stack().set('submit_to', '[e-mail]')
-        self.run_bzr_error(('Unknown mail client: bogus',),
+        self.run_bzr_error(('Bad value "bogus" for option "mail_client"',),
                            'send -f branch')
 
     def test_mailto_child_option(self):
         """Make sure that child_submit_to is used."""
         b = branch.Branch.open('branch')
-        b.get_config().set_user_option('mail_client', 'bogus')
+        b.get_config_stack().set('mail_client', 'bogus')
         parent = branch.Branch.open('parent')
         parent.get_config_stack().set('child_submit_to', '[e-mail]')
-        self.run_bzr_error(('Unknown mail client: bogus',), 'send -f branch')
+        self.run_bzr_error(('Bad value "bogus" for option "mail_client"',),
+                'send -f branch')
 
     def test_format(self):
         md = self.get_MD(['--format=4'])

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-01-27 21:25:21 +0000
+++ bzrlib/tests/test_config.py	2012-02-02 12:15:24 +0000
@@ -1904,48 +1904,6 @@
             location='http://example.com/specific')
         self.assertEqual(my_config.get_user_option('option'), 'exact')
 
-    def test_get_mail_client(self):
-        config = self.get_branch_config()
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.DefaultMail)
-
-        # Specific clients
-        config.set_user_option('mail_client', 'evolution')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.Evolution)
-
-        config.set_user_option('mail_client', 'kmail')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.KMail)
-
-        config.set_user_option('mail_client', 'mutt')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.Mutt)
-
-        config.set_user_option('mail_client', 'thunderbird')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.Thunderbird)
-
-        # Generic options
-        config.set_user_option('mail_client', 'default')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.DefaultMail)
-
-        config.set_user_option('mail_client', 'editor')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.Editor)
-
-        config.set_user_option('mail_client', 'mapi')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.MAPIClient)
-
-        config.set_user_option('mail_client', 'xdg-email')
-        client = config.get_mail_client()
-        self.assertIsInstance(client, mail_client.XDGEmail)
-
-        config.set_user_option('mail_client', 'firebird')
-        self.assertRaises(errors.UnknownMailClient, config.get_mail_client)
-
 
 class TestMailAddressExtraction(tests.TestCase):
 
@@ -4889,3 +4847,56 @@
         self.overrideEnv('BZR_EMAIL', None)
         self.overrideEnv('EMAIL', '[e-mail]')
         self.assertEquals('[e-mail]', conf.get('email'))
+
+
+class MailClientOptionTests(tests.TestCase):
+
+    def test_default(self):
+        conf = config.MemoryStack('')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.DefaultMail)
+
+    def test_evolution(self):
+        conf = config.MemoryStack('mail_client=evolution')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.Evolution)
+
+    def test_kmail(self):
+        conf = config.MemoryStack('mail_client=kmail')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.KMail)
+
+    def test_mutt(self):
+        conf = config.MemoryStack('mail_client=mutt')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.Mutt)
+
+    def test_thunderbird(self):
+        conf = config.MemoryStack('mail_client=thunderbird')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.Thunderbird)
+
+    def test_explicit_default(self):
+        conf = config.MemoryStack('mail_client=default')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.DefaultMail)
+
+    def test_editor(self):
+        conf = config.MemoryStack('mail_client=editor')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.Editor)
+
+    def test_mapi(self):
+        conf = config.MemoryStack('mail_client=mapi')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.MAPIClient)
+
+    def test_xdg_email(self):
+        conf = config.MemoryStack('mail_client=xdg-email')
+        client = conf.get('mail_client')
+        self.assertIs(client, mail_client.XDGEmail)
+
+    def test_unknown(self):
+        conf = config.MemoryStack('mail_client=firebird')
+        self.assertRaises(errors.ConfigOptionValueError, conf.get,
+                'mail_client')

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-01-27 21:11:36 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-02-02 12:15:24 +0000
@@ -52,6 +52,9 @@
   or tuples of bytestrings.
   (Jelmer Vernooij)
 
+* ``mail_client`` now accepts a configuration stack object rather than
+  an old style Config object. (Jelmer Vernooij)
+
 * New configuration option class ``RegistryOption`` which is backed
   onto a registry. (Jelmer Vernooij)