Merge pull request #52 from pubs/feat/better-exceptions

Better exceptions handling at the ui level.

pubs/commands/add_cmd.py

@@ -104,11 +104,7 @@ def command(conf, args):
         ui.error('citekey already exist {}.'.format(citekey))
         ui.exit(1)
 
-    try:
-        p = paper.Paper.from_bibentry(bibentry, citekey=citekey)
-    except Exception as e:
-        ui.error(e.args[0])
-        ui.exit(1)
+    p = paper.Paper.from_bibentry(bibentry, citekey=citekey)
 
     # tags
 
@@ -132,21 +128,16 @@ def command(conf, args):
     if move is None:
         move = conf['main']['doc_add'] == 'move'
 
-    try:
-        rp.push_paper(p)
-        ui.message('added to pubs:\n{}'.format(pretty.paper_oneliner(p)))
-        if docfile is not None:
-            rp.push_doc(p.citekey, docfile, copy=copy or args.move)
-            if copy:
-                if move:
-                    content.remove_file(docfile)
-
-            if copy:
-                if move:
-                    ui.message('{} was moved to the pubs repository.'.format(docfile))
-                else:
-                    ui.message('{} was copied to the pubs repository.'.format(docfile))
-
-    except ValueError as v:
-        ui.error(v.message)
-        ui.exit(1)
+    rp.push_paper(p)
+    ui.message('added to pubs:\n{}'.format(pretty.paper_oneliner(p)))
+    if docfile is not None:
+        rp.push_doc(p.citekey, docfile, copy=copy or args.move)
+        if copy:
+            if move:
+                content.remove_file(docfile)
+
+        if copy:
+            if move:
+                ui.message('{} was moved to the pubs repository.'.format(docfile))
+            else:
+                ui.message('{} was copied to the pubs repository.'.format(docfile))

pubs/commands/doc_cmd.py

@@ -65,26 +65,19 @@ def command(conf, args):
             if not ui.input_yn(question=msg, default='n'):
                 ui.exit(0)
             else:
-                try:
-                    rp.remove_doc(paper.citekey)
-                except (ValueError, IOError) as v:
-                    ui.error(v.message)
-                    ui.exit(1)
+                rp.remove_doc(paper.citekey)
 
-        try:
-            document = args.document[0]
-            if args.link:
-                rp.push_doc(paper.citekey, document, copy=False)
-            else:
-                rp.push_doc(paper.citekey, document, copy=True)
-            if not args.link and args.move:
-                content.remove_file(document)
-
-            ui.message('{} added to {}'.format(color.dye_out(document, 'filepath'),
-                                                  color.dye_out(paper.citekey, 'citekey')))
-        except (ValueError, IOError) as v:
-            ui.error(v.message)
-            ui.exit(1)
+        document = args.document[0]
+        if args.link:
+            rp.push_doc(paper.citekey, document, copy=False)
+        else:
+            rp.push_doc(paper.citekey, document, copy=True)
+        if not args.link and args.move:
+            content.remove_file(document)
+
+        ui.message('{} added to {}'.format(
+            color.dye_out(document, 'filepath'),
+            color.dye_out(paper.citekey, 'citekey')))
 
     elif args.action == 'remove':
 
@@ -103,11 +96,7 @@ def command(conf, args):
                 if not ui.input_yn(question=msg, default='n'):
                     continue
 
-            try:
-                rp.remove_doc(paper.citekey)
-            except (ValueError, IOError) as v:
-                ui.error(v.message)
-                ui.exit(1)
+            rp.remove_doc(paper.citekey)
 
     elif args.action == 'export':
 
@@ -133,7 +122,7 @@ def command(conf, args):
                     dest_path = path + os.path.basename(real_doc_path)
                     content.copy_content(real_doc_path, dest_path)
             except (ValueError, IOError) as e:
-                ui.error(e.message)
+                ui.error(str(e))
 
     elif args.action == 'open':
         with_command = args.cmd

pubs/commands/export_cmd.py

@@ -21,21 +21,17 @@ def command(conf, args):
     ui = get_ui()
     rp = repo.Repository(conf)
 
-    try:
-        papers = []
-        if len(args.citekeys) < 1:
-            papers = rp.all_papers()
-        else:
-            for key in resolve_citekey_list(repo=rp, citekeys=args.citekeys, ui=ui, exit_on_fail=True):
-                papers.append(rp.pull_paper(key))
-
-        bib = {}
-        for p in papers:
-            bib[p.citekey] = p.bibdata
-
-        exporter = endecoder.EnDecoder()
-        bibdata_raw = exporter.encode_bibdata(bib)
-        ui.message(bibdata_raw)
-
-    except Exception as e:
-        ui.error(e.message)
+    papers = []
+    if len(args.citekeys) < 1:
+        papers = rp.all_papers()
+    else:
+        for key in resolve_citekey_list(repo=rp, citekeys=args.citekeys, ui=ui, exit_on_fail=True):
+            papers.append(rp.pull_paper(key))
+
+    bib = {}
+    for p in papers:
+        bib[p.citekey] = p.bibdata
+
+    exporter = endecoder.EnDecoder()
+    bibdata_raw = exporter.encode_bibdata(bib)
+    ui.message(bibdata_raw)

pubs/commands/import_cmd.py

@@ -8,7 +8,7 @@ from .. import color
 from ..paper import Paper
 
 from ..uis import get_ui
-from ..content import system_path, read_file
+from ..content import system_path, read_text_file
 
 
 def parser(subparsers):
@@ -45,7 +45,7 @@ def many_from_path(bibpath):
 
     biblist = []
     for filepath in all_files:
-        biblist.append(coder.decode_bibdata(read_file(filepath)))
+        biblist.append(coder.decode_bibdata(read_text_file(filepath)))
 
     papers = {}
     for b in biblist:
@@ -74,20 +74,15 @@ def command(conf, args):
     papers = many_from_path(bibpath)
     keys = args.keys or papers.keys()
     for k in keys:
-        try:
-            p = papers[k]
-            if isinstance(p, Exception):
-                ui.error(u'Could not load entry for citekey {}.'.format(k))
+        p = papers[k]
+        if isinstance(p, Exception):
+            ui.error(u'Could not load entry for citekey {}.'.format(k))
+        else:
+            rp.push_paper(p)
+            ui.info(u'{} imported.'.format(color.dye_out(p.citekey, 'citekey')))
+            docfile = bibstruct.extract_docfile(p.bibdata)
+            if docfile is None:
+                ui.warning("No file for {}.".format(p.citekey))
             else:
-                rp.push_paper(p)
-                ui.info(u'{} imported.'.format(color.dye_out(p.citekey, 'citekey')))
-                docfile = bibstruct.extract_docfile(p.bibdata)
-                if docfile is None:
-                    ui.warning("No file for {}.".format(p.citekey))
-                else:
-                    rp.push_doc(p.citekey, docfile, copy=copy)
-                    #FIXME should move the file if configured to do so.
-        except KeyError:
-            ui.error(u'No entry found for citekey {}.'.format(k))
-        except IOError as e:
-            ui.error(e.message)
+                rp.push_doc(p.citekey, docfile, copy=copy)
+                #FIXME should move the file if configured to do so.

pubs/commands/note_cmd.py

@@ -19,8 +19,5 @@ def command(conf, args):
     ui = get_ui()
     rp = repo.Repository(conf)
     citekey = resolve_citekey(rp, args.citekey, ui=ui, exit_on_fail=True)
-    try:
-        notepath = rp.databroker.real_notepath(citekey)
-        content.edit_file(conf['main']['edit_cmd'], notepath, temporary=False)
-    except Exception as e:
-        ui.error(e.message)
+    notepath = rp.databroker.real_notepath(citekey)
+    content.edit_file(conf['main']['edit_cmd'], notepath, temporary=False)

pubs/commands/remove_cmd.py

@@ -26,13 +26,17 @@ def command(conf, args):
             .format(', '.join([color.dye_out(c, 'citekey') for c in args.citekeys])))
         sure = ui.input_yn(question=are_you_sure, default='n')
     if force or sure:
+        failed = False  # Whether something failed
         for c in keys:
             try:
                 rp.remove_paper(c)
             except Exception as e:
                 ui.error(e.message)
+                failed = True
         ui.message('The publication(s) [{}] were removed'.format(
             ', '.join([color.dye_out(c, 'citekey') for c in keys])))
+        if failed:
+            ui.exit()  # Exit with nonzero error code
         # FIXME: print should check that removal proceeded well.
     else:
         ui.message('The publication(s) [{}] were {} removed'.format(

pubs/commands/rename_cmd.py

@@ -21,9 +21,6 @@ def command(conf, args):
     rp = repo.Repository(conf)
 
     # TODO: here should be a test whether the new citekey is valid
-    try:
-        key = resolve_citekey(repo=rp, citekey=args.citekey, ui=ui, exit_on_fail=True)
-        paper = rp.pull_paper(key)
-        rp.rename_paper(paper, args.new_citekey)
-    except Exception as e:
-        ui.error(e.message)
+    key = resolve_citekey(repo=rp, citekey=args.citekey, ui=ui, exit_on_fail=True)
+    paper = rp.pull_paper(key)
+    rp.rename_paper(paper, args.new_citekey)

pubs/config/spec.py

@@ -22,6 +22,9 @@ open_cmd = string(default=None)
 # "kate --block"
 edit_cmd = string(default='')
 
+# If true debug mode is on which means exceptions are not catched and
+# the full python stack is printed.
+debug = boolean(default=False)
 
 [formating]
 

pubs/content.py

@@ -14,6 +14,18 @@ from .p3 import urlparse, HTTPConnection, urlopen
       be prefixed by 'byte_'
 """
 
+
+class UnableToDecodeTextFile(Exception):
+
+    _msg = "unknown encoding (maybe not a text file) for: {}"
+
+    def __init__(self, path):
+        self.path = path
+
+    def __str__(self):
+        return self._msg.format(self.path)
+
+
 # files i/o
 
 def _check_system_path_exists(path, fail=True):
@@ -56,10 +68,14 @@ def check_directory(path, fail=True):
             and _check_system_path_is(u'isdir', syspath, fail=fail))
 
 
-def read_file(filepath):
+def read_text_file(filepath):
     check_file(filepath)
-    with _open(filepath, 'r') as f:
-        content = f.read()
+    try:
+        with _open(filepath, 'r') as f:
+            content = f.read()
+    except UnicodeDecodeError:
+        raise UnableToDecodeTextFile(filepath)
+        # Should "raise from". TODO once python 2 is droped.
     return content
 
 
@@ -120,7 +136,7 @@ def get_content(path, ui=None):
     if content_type(path) == u'url':
         return _get_byte_url_content(path, ui=ui).decode(encoding='utf-8')
     else:
-        return read_file(path)
+        return read_text_file(path)
 
 
 def move_content(source, target, overwrite=False):
@@ -155,7 +171,7 @@ def editor_input(editor, initial=u'', suffix='.tmp'):
     cmd = shlex.split(editor)  # this enable editor command with option, e.g. gvim -f
     cmd.append(tfile_name)
     subprocess.call(cmd)
-    content = read_file(tfile_name)
+    content = read_text_file(tfile_name)
     os.remove(tfile_name)
     return content
 
@@ -163,7 +179,7 @@ def editor_input(editor, initial=u'', suffix='.tmp'):
 def edit_file(editor, path_to_file, temporary=True):
     if temporary:
         check_file(path_to_file, fail=True)
-        content = read_file(path_to_file)
+        content = read_text_file(path_to_file)
         content = editor_input(editor, content)
         write_file(path_to_file, content)
     else:

pubs/filebroker.py

@@ -2,7 +2,7 @@ import os
 import re
 from .p3 import urlparse
 
-from .content import (check_file, check_directory, read_file, write_file,
+from .content import (check_file, check_directory, read_text_file, write_file,
                       system_path, check_content, content_type, get_content,
                       copy_content)
 
@@ -43,11 +43,11 @@ class FileBroker(object):
 
     def pull_metafile(self, citekey):
         filepath = os.path.join(self.metadir, citekey + '.yaml')
-        return read_file(filepath)
+        return read_text_file(filepath)
 
     def pull_bibfile(self, citekey):
         filepath = os.path.join(self.bibdir, citekey + '.bib')
-        return read_file(filepath)
+        return read_text_file(filepath)
 
     def push_metafile(self, citekey, metadata):
         """Put content to disk. Will gladly override anything standing in its way."""

pubs/pubs_cmd.py

@@ -32,56 +32,56 @@ CORE_CMDS = collections.OrderedDict([
 
 def execute(raw_args=sys.argv):
 
-    conf_parser = argparse.ArgumentParser(prog="pubs", add_help=False)
-    conf_parser.add_argument("-c", "--config", help="path to config file",
-                             type=str, metavar="FILE")
-    conf_parser.add_argument('--force-colors', dest='force_colors',
-                             action='store_true', default=False,
-                             help='color are not disabled when piping to a file or other commands')
-    #conf_parser.add_argument("-u", "--update", help="update config if needed",
-    #                         default=False, action='store_true')
-    top_args, remaining_args = conf_parser.parse_known_args(raw_args[1:])
-
-    if top_args.config:
-        conf_path = top_args.config
-    else:
-        conf_path = config.get_confpath(verify=False)  # will be checked on load
-
-    # Loading config
-    if len(remaining_args) > 0 and remaining_args[0] != 'init':
-        try:
+    try:
+        conf_parser = argparse.ArgumentParser(prog="pubs", add_help=False)
+        conf_parser.add_argument("-c", "--config", help="path to config file",
+                                 type=str, metavar="FILE")
+        conf_parser.add_argument('--force-colors', dest='force_colors',
+                                 action='store_true', default=False,
+                                 help='color are not disabled when piping to a file or other commands')
+        #conf_parser.add_argument("-u", "--update", help="update config if needed",
+        #                         default=False, action='store_true')
+        top_args, remaining_args = conf_parser.parse_known_args(raw_args[1:])
+
+        if top_args.config:
+            conf_path = top_args.config
+        else:
+            conf_path = config.get_confpath(verify=False)  # will be checked on load
+
+        # Loading config
+        if len(remaining_args) > 0 and remaining_args[0] != 'init':
             conf = config.load_conf(path=conf_path, check=False)
             if update.update_check(conf, path=conf.filename):
                 # an update happened, reload conf.
                 conf = config.load_conf(path=conf_path, check=False)
             config.check_conf(conf)
-        except IOError as e:
-            print('error: {}'.format(str(e)))
-            sys.exit()
-    else:
-        conf = config.load_default_conf()
-        conf.filename = conf_path
-
-    uis.init_ui(conf, force_colors=top_args.force_colors)
-    ui = uis.get_ui()
-
-    parser = argparse.ArgumentParser(description="research papers repository",
-                                     prog="pubs", add_help=True)
-    parser.add_argument('--version', action='version', version=__version__)
-    subparsers = parser.add_subparsers(title="valid commands", dest="command")
-    subparsers.required = True
-
-    # Populate the parser with core commands
-    for cmd_name, cmd_mod in CORE_CMDS.items():
-        cmd_parser = cmd_mod.parser(subparsers)
-        cmd_parser.set_defaults(func=cmd_mod.command)
-
-    # Extend with plugin commands
-    plugins.load_plugins(conf, ui)
-    for p in plugins.get_plugins().values():
-        p.update_parser(subparsers)
-
-    # Parse and run appropriate command
-    args = parser.parse_args(remaining_args)
-    args.prog = "pubs" # FIXME?
-    args.func(conf, args)
+        else:
+            conf = config.load_default_conf()
+            conf.filename = conf_path
+
+        uis.init_ui(conf, force_colors=top_args.force_colors)
+        ui = uis.get_ui()
+
+        parser = argparse.ArgumentParser(description="research papers repository",
+                                         prog="pubs", add_help=True)
+        parser.add_argument('--version', action='version', version=__version__)
+        subparsers = parser.add_subparsers(title="valid commands", dest="command")
+        subparsers.required = True
+
+        # Populate the parser with core commands
+        for cmd_name, cmd_mod in CORE_CMDS.items():
+            cmd_parser = cmd_mod.parser(subparsers)
+            cmd_parser.set_defaults(func=cmd_mod.command)
+
+        # Extend with plugin commands
+        plugins.load_plugins(conf, ui)
+        for p in plugins.get_plugins().values():
+            p.update_parser(subparsers)
+
+        # Parse and run appropriate command
+        args = parser.parse_args(remaining_args)
+        args.prog = "pubs"  # FIXME?
+        args.func(conf, args)
+
+    except Exception as e:
+        uis.get_ui().handle_exception(e)

pubs/repo.py

@@ -12,12 +12,26 @@ def _base27(n):
     return _base27((n - 1) // 26) + chr(ord('a') + ((n - 1) % 26)) if n else ''
 
 
-class CiteKeyCollision(Exception):
-    pass
+class CiteKeyError(Exception):
 
+    default_message = "Wrong citekey: {}."
 
-class InvalidReference(Exception):
-    pass
+    def __init__(self, citekey, message=None):
+        self.message = message
+        self.citekey = citekey
+
+    def __str__(self):
+        return self.message or self.default_msg.format(self.citekey)
+
+
+class CiteKeyCollision(CiteKeyError):
+
+    default_message = "Citekey already in use: {}."
+
+
+class CiteKeyNotFound(CiteKeyError):
+
+    default_message = "No entry found for citekey: {}."
 
 
 class Repository(object):
@@ -63,7 +77,7 @@ class Repository(object):
                 citekey=citekey,
                 metadata=self.databroker.pull_metadata(citekey))
         else:
-            raise InvalidReference('{} citekey not found'.format(citekey))
+            raise CiteKeyNotFound(citekey)
 
     def push_paper(self, paper, overwrite=False, event=True):
         """ Push a paper to disk
@@ -73,7 +87,7 @@ class Repository(object):
         """
         bibstruct.check_citekey(paper.citekey)
         if (not overwrite) and (paper.citekey in self):
-            raise CiteKeyCollision('citekey {} already in use'.format(paper.citekey))
+            raise CiteKeyCollision(paper.citekey)
         if not paper.added:
             paper.added = datetime.now()
         self.databroker.push_bibentry(paper.citekey, paper.bibentry)
@@ -130,7 +144,8 @@ class Repository(object):
         else:
             # check if new_citekey does not exists
             if new_citekey in self:
-                raise CiteKeyCollision("can't rename paper to {}, conflicting files exists".format(new_citekey))
+                msg = "Can't rename paper to {}, citekey already exists.".format(new_citekey)
+                raise CiteKeyCollision(new_citekey, message=msg)
 
             # move doc file if necessary
             if self.databroker.in_docsdir(paper.docpath):

pubs/uis.py

@@ -62,6 +62,7 @@ class PrintUI(object):
                                                         errors='replace')
         self._stderr  = codecs.getwriter(self.encoding)(_get_raw_stderr(),
                                                         errors='replace')
+        self.debug = conf['main'].get('debug', False)
 
     def message(self, *messages, **kwargs):
         kwargs['file'] = self._stdout
@@ -82,6 +83,13 @@ class PrintUI(object):
     def exit(self, error_code=1):
         sys.exit(error_code)
 
+    def handle_exception(self, exc):
+        if self.debug:
+            raise exc
+        else:
+            self.error(ustr(exc))
+            self.exit()
+
 
 class InputUI(PrintUI):
     """UI class. Stores configuration parameters and system information.

tests/test_filebroker.py

@@ -29,10 +29,10 @@ class TestFileBroker(fake_env.TestFakeFs):
         fake_env.copy_dir(self.fs, os.path.join(os.path.dirname(__file__), 'testrepo'), 'testrepo')
         fb = filebroker.FileBroker('testrepo', create = True)
 
-        bib_content = content.read_file('testrepo/bib/Page99.bib')
+        bib_content = content.read_text_file('testrepo/bib/Page99.bib')
         self.assertEqual(fb.pull_bibfile('Page99'), bib_content)
 
-        meta_content = content.read_file('testrepo/meta/Page99.yaml')
+        meta_content = content.read_text_file('testrepo/meta/Page99.yaml')
         self.assertEqual(fb.pull_metafile('Page99'), meta_content)
 
     def test_errors(self):

tests/test_repo.py

@@ -5,7 +5,7 @@ import dotdot
 import fake_env
 import fixtures
 
-from pubs.repo import Repository, _base27, CiteKeyCollision, InvalidReference
+from pubs.repo import Repository, _base27, CiteKeyCollision, CiteKeyNotFound
 from pubs.paper import Paper
 from pubs import config
 
@@ -77,7 +77,7 @@ class TestUpdatePaper(TestRepo):
     def test_update_new_key_removes_old(self):
         paper = self.repo.pull_paper('turing1950computing')
         self.repo.rename_paper(paper, 'Turing1950')
-        with self.assertRaises(InvalidReference):
+        with self.assertRaises(CiteKeyNotFound):
             self.repo.pull_paper('turing1950computing')
         self.assertNotIn('turing1950computing', self.repo)
 

tests/test_usecase.py

@@ -140,6 +140,10 @@ class TestInit(CommandTestCase):
         self.assertEqual(set(self.fs['os'].listdir(pubsdir)),
                          {'bib', 'doc', 'meta', 'notes'})
 
+    def test_init_config(self):
+        self.execute_cmds(['pubs init'])
+        self.assertTrue(self.fs['os'].path.isfile(self.default_conf_path))
+
 
 class TestAdd(DataCommandTestCase):
 
@@ -212,15 +216,13 @@ class TestAdd(DataCommandTestCase):
         with self.assertRaises(SystemExit):
             self.execute_cmds(cmds)
 
-    @unittest.expectedFailure
-    def test_leading_citekey_space(self):
-        cmds = ['pubs init',
-                'pubs add /data/leadingspace.bib',
-                'pubs rename LeadingSpace NoLeadingSpace',
-                ]
-        self.execute_cmds(cmds)
-
-
+# To be fixed
+#    def test_leading_citekey_space(self):
+#        cmds = ['pubs init',
+#                'pubs add /data/leadingspace.bib',
+#                'pubs rename LeadingSpace NoLeadingSpace',
+#                ]
+#        self.execute_cmds(cmds)
 
 
 class TestList(DataCommandTestCase):
@@ -538,7 +540,6 @@ class TestUsecase(DataCommandTestCase):
                 'pubs doc add --move data/pagerank.pdf Page99'
                ]
         self.execute_cmds(cmds)
-        self.assertTrue(self.fs['os'].path.isfile(self.default_conf_path))
         self.assertFalse(self.fs['os'].path.exists('/data/pagerank.pdf'))
 
     def test_doc_remove(self):