Merge pull request #1181 from three-comrades/compose_err

Give better feedback on invalid user input.

8 files changed, 88 insertions, 21 deletions

diff --git a/alot/__main__.py b/alot/__main__.py
index fc34164f..ee5e442a 100644
--- a/alot/__main__.py
+++ b/alot/__main__.py
@@ -106,6 +106,8 @@ def main():
         settings.read_config()
         settings.read_notmuch_config()
     except (ConfigError, OSError, IOError) as e:
+        print('Error when parsing a config file. '
+              'See log for potential details.')
         sys.exit(e)
 
     # store options given by config swiches to the settingsManager:
diff --git a/alot/account.py b/alot/account.py
index a4ce439a..c672686d 100644
--- a/alot/account.py
+++ b/alot/account.py
@@ -359,11 +359,11 @@ class SendmailAccount(Account):
         def errb(failure):
             """The callback used on error."""
             termobj = failure.value
-            errmsg = '%s failed with code %s:\n%s' % \
-                (self.cmd, termobj.exitCode, str(failure.value))
+            errmsg = '%s failed with code %s:\n%s\nProcess stderr:\n%s' % \
+                     (self.cmd, termobj.exitCode, str(failure.value),
+                      failure.value.stderr)
             logging.error(errmsg)
             logging.error(failure.getTraceback())
-            logging.error(failure.value.stderr)
             raise SendingMailFailed(errmsg)
 
         # make sure self.mail is a string
diff --git a/alot/commands/globals.py b/alot/commands/globals.py
index 57ddfcb6..bf1cad01 100644
--- a/alot/commands/globals.py
+++ b/alot/commands/globals.py
@@ -35,6 +35,7 @@ from ..widgets.utils import DialogBox
 from ..db.errors import DatabaseLockedError
 from ..db.envelope import Envelope
 from ..settings.const import settings
+from ..settings.errors import ConfigError, NoMatchingAccount
 from ..utils import argparse as cargparse
 
 MODE = 'global'
@@ -816,7 +817,14 @@ class ComposeCommand(Command):
         # find out the right account
         sender = self.envelope.get('From')
         name, addr = email.utils.parseaddr(sender)
-        account = settings.get_account_by_address(addr)
+        try:
+            account = settings.get_account_by_address(addr)
+        except NoMatchingAccount:
+            msg = 'Cannot compose mail - no account found for `%s`' % addr
+            logging.error(msg)
+            ui.notify(msg, priority='error')
+            raise CommandCanceled()
+
         if account is None:
             accounts = settings.get_accounts()
             if not accounts:
@@ -965,4 +973,8 @@ class ReloadCommand(Command):
     """Reload configuration."""
 
     def apply(self, ui):
-        settings.reload()
+        try:
+            settings.reload()
+        except ConfigError as e:
+            ui.notify('Error when reloading config files:\n {}'.format(e),
+                      priority='error')
diff --git a/alot/settings/manager.py b/alot/settings/manager.py
index 98d534fb..dffe1b32 100644
--- a/alot/settings/manager.py
+++ b/alot/settings/manager.py
@@ -79,7 +79,7 @@ class SettingsManager(object):
         """parse alot's config file from path"""
         spec = os.path.join(DEFAULTSPATH, 'alot.rc.spec')
         newconfig = read_config(
-            self.alot_rc_path, spec, checks={
+            self.alot_rc_path, spec, report_extra=True, checks={
                 'mail_container': checks.mail_container,
                 'force_list': checks.force_list,
                 'align': checks.align_mode,
@@ -116,22 +116,19 @@ class SettingsManager(object):
             # tl/dr; If the loop loads a theme it breaks. If it doesn't break,
             # then it raises a ConfigError.
             for dir_ in itertools.chain([themes_dir], data_dirs):
-                if not os.path.isdir(dir_):
-                    logging.warning(
-                        'cannot find theme %s: themes_dir %s is missing',
-                        themestring, dir_)
+                theme_path = os.path.join(dir_, themestring)
+                if not os.path.exists(os.path.expanduser(theme_path)):
+                    logging.warning('Theme `%s` does not exist.', theme_path)
                 else:
-                    theme_path = os.path.join(dir_, themestring)
                     try:
                         self._theme = Theme(theme_path)
                     except ConfigError as e:
-                        logging.warning(
-                            'Theme file %s failed validation: %s',
-                            themestring, e)
+                        raise ConfigError('Theme file `%s` failed '
+                                          'validation:\n%s' % (theme_path, e))
                     else:
                         break
             else:
-                raise ConfigError('Cannot load theme {}, see log for more '
+                raise ConfigError('Could not find theme {}, see log for more '
                                   'information'.format(themestring))
 
         # if still no theme is set, resort to default
diff --git a/alot/settings/theme.py b/alot/settings/theme.py
index f2a59763..d74cb0e7 100644
--- a/alot/settings/theme.py
+++ b/alot/settings/theme.py
@@ -22,7 +22,7 @@ class Theme(object):
         :raises: :class:`~alot.settings.errors.ConfigError`
         """
         self._spec = os.path.join(DEFAULTSPATH, 'theme.spec')
-        self._config = read_config(path, self._spec,
+        self._config = read_config(path, self._spec, report_extra=True,
                                    checks={'align': checks.align_mode,
                                            'widthtuple': checks.width_tuple,
                                            'force_list': checks.force_list,
diff --git a/alot/settings/utils.py b/alot/settings/utils.py
index d87157c3..8d656b79 100644
--- a/alot/settings/utils.py
+++ b/alot/settings/utils.py
@@ -3,14 +3,18 @@
 # For further details see the COPYING file
 from __future__ import absolute_import
 
-from configobj import ConfigObj, ConfigObjError, flatten_errors
+import logging
+
+from configobj import (ConfigObj, ConfigObjError, flatten_errors,
+                       get_extra_values)
 from validate import Validator
 from urwid import AttrSpec
 
 from .errors import ConfigError
 
 
-def read_config(configpath=None, specpath=None, checks=None):
+def read_config(configpath=None, specpath=None, checks=None,
+                report_extra=False):
     """
     get a (validated) config object for given config file path.
 
@@ -21,6 +25,8 @@ def read_config(configpath=None, specpath=None, checks=None):
     :param checks: custom checks to use for validator.
         see `validate docs <http://www.voidspace.org.uk/python/validate.html>`_
     :type checks: dict str->callable,
+    :param report_extra: log if a setting is not present in the spec file
+    :type report_extra: boolean
     :raises: :class:`~alot.settings.errors.ConfigError`
     :rtype: `configobj.ConfigObj`
     """
@@ -30,7 +36,9 @@ def read_config(configpath=None, specpath=None, checks=None):
         config = ConfigObj(infile=configpath, configspec=specpath,
                            file_error=True, encoding='UTF8')
     except ConfigObjError as e:
-        raise ConfigError(e)
+        msg = 'Error when parsing `%s`:\n%s' % (configpath, e)
+        logging.error(msg)
+        raise ConfigError(msg)
     except IOError:
         raise ConfigError('Could not read %s and/or %s'
                           % (configpath, specpath))
@@ -61,6 +69,18 @@ def read_config(configpath=None, specpath=None, checks=None):
                     msg = 'section "%s" is missing' % '.'.join(section_list)
                 error_msg += msg + '\n'
             raise ConfigError(error_msg)
+
+        extra_values = get_extra_values(config) if report_extra else None
+        if extra_values:
+            msg = ['Unknown values were found in `%s`. Please check for '
+                   'typos if a specified setting does not seem to work:'
+                   % configpath]
+            for sections, val in extra_values:
+                if sections:
+                    msg.append('%s: %s' % ('->'.join(sections), val))
+                else:
+                    msg.append(str(val))
+            logging.info('\n'.join(msg))
     return config
 
 
diff --git a/tests/commands/envelope_test.py b/tests/commands/envelope_test.py
index 55cb61cb..c1b928db 100644
--- a/tests/commands/envelope_test.py
+++ b/tests/commands/envelope_test.py
@@ -309,7 +309,7 @@ class TestSignCommand(unittest.TestCase):
                 [[default]]
                     realname = foo
                     address = foo@example.com
-                    sendmail_commnd = /bin/true
+                    sendmail_command = /bin/true
             """)
 
         # Allow settings.reload to work by not deleting the file until the end
diff --git a/tests/settings/manager_test.py b/tests/settings/manager_test.py
index 975bcb0e..eb2f37fd 100644
--- a/tests/settings/manager_test.py
+++ b/tests/settings/manager_test.py
@@ -11,6 +11,8 @@ import tempfile
 import textwrap
 import unittest
 
+import mock
+
 from alot.settings.manager import SettingsManager
 from alot.settings.errors import ConfigError, NoMatchingAccount
 
@@ -80,6 +82,40 @@ class TestSettingsManager(unittest.TestCase):
 
         manager.get_theming_attribute('global', 'body')
 
+    def test_unknown_settings_in_config_are_logged(self):
+        # todo: For py3, don't mock the logger, use assertLogs
+        unknown_settings = ['templates_dir', 'unknown_section', 'unknown_1',
+                            'unknown_2']
+        with tempfile.NamedTemporaryFile(delete=False) as f:
+            f.write(textwrap.dedent("""\
+                {x[0]} = /templates/dir
+                [{x[1]}]
+                    # Values in unknown sections are not reported.
+                    barfoo = barfoo
+                [tags]
+                    [[foobar]]
+                        {x[2]} = baz
+                        translated = translation
+                        {x[3]} = bar
+                """.format(x=unknown_settings)))
+        self.addCleanup(os.unlink, f.name)
+
+        with mock.patch('alot.settings.utils.logging') as mock_logger:
+            SettingsManager(alot_rc=f.name)
+            success = False
+            for call_args in mock_logger.info.call_args_list:
+                msg = call_args[0][0]
+                if all([s in msg for s in unknown_settings]):
+                    success = True
+                    break
+            else:
+                print('Could not find all unknown settings in logging.info.\n'
+                      'Unknown settings:')
+                print(unknown_settings)
+                print('Calls to mocked logging.info:')
+                print(mock_logger.info.call_args_list)
+        self.assertTrue(success)
+
     def test_read_notmuch_config_doesnt_exist(self):
         with tempfile.NamedTemporaryFile(delete=False) as f:
             f.write(textwrap.dedent("""\
@@ -117,7 +153,7 @@ class TestSettingsManagerGetAccountByAddress(utilities.TestCaseClassCleanup):
                 [[default]]
                     realname = That Guy
                     address = that_guy@example.com
-                    sendmail_commnd = /bin/true
+                    sendmail_command = /bin/true
 
                 [[other]]
                     realname = A Dude