diff --git a/beets/autotag/hooks.py b/beets/autotag/hooks.py index 78045cd3c..e8ec0a832 100644 --- a/beets/autotag/hooks.py +++ b/beets/autotag/hooks.py @@ -118,14 +118,14 @@ def _album_for_id(album_id): try: return mb.album_for_id(album_id) except mb.MusicBrainzAPIError as exc: - log.error(exc.log()) + exc.log(log) def _track_for_id(track_id): """Get an item for a recording MBID.""" try: return mb.track_for_id(track_id) except mb.MusicBrainzAPIError as exc: - log.error(exc.log()) + exc.log(log) def _album_candidates(items, artist, album, va_likely): """Search for album matches. ``items`` is a list of Item objects @@ -141,14 +141,14 @@ def _album_candidates(items, artist, album, va_likely): try: out.extend(mb.match_album(artist, album, len(items))) except mb.MusicBrainzAPIError as exc: - log.error(exc.log()) + exc.log(log) # Also add VA matches from MusicBrainz where appropriate. if va_likely and album: try: out.extend(mb.match_album(None, album, len(items))) except mb.MusicBrainzAPIError as exc: - log.error(exc.log()) + exc.log(log) # Candidates from plugins. out.extend(plugins.candidates(items)) @@ -167,7 +167,7 @@ def _item_candidates(item, artist, title): try: out.extend(mb.match_track(artist, title)) except mb.MusicBrainzAPIError as exc: - log.error(exc.log()) + exc.log(log) # Plugin candidates. out.extend(plugins.item_candidates(item)) diff --git a/beets/autotag/mb.py b/beets/autotag/mb.py index 47188f456..b6edd760a 100644 --- a/beets/autotag/mb.py +++ b/beets/autotag/mb.py @@ -16,9 +16,11 @@ """ import logging import musicbrainzngs +import traceback import beets.autotag.hooks import beets +from beets import util SEARCH_LIMIT = 5 VARIOUS_ARTISTS_ID = '89ad4ac3-39f7-470e-963a-56509c546377' @@ -26,21 +28,18 @@ VARIOUS_ARTISTS_ID = '89ad4ac3-39f7-470e-963a-56509c546377' musicbrainzngs.set_useragent('beets', beets.__version__, 'http://beets.radbox.org/') -class MusicBrainzAPIError(Exception): - """An error while talking to MusicBrainz. Has three fields: - `reason`, the underlying exception; `verb`, the action being taken - (a string); and `query`, the parameter to the action (of any type). +class MusicBrainzAPIError(util.HumanReadableException): + """An error while talking to MusicBrainz. The `query` field is the + parameter to the action and may have any type. """ - def __init__(self, reason, verb, query): - self.reason = reason - self.verb = verb + def __init__(self, reason, verb, query, tb=None): self.query = query - msg = u'"{0}" in {1} with query {2}'.format(reason, verb, repr(query)) - super(MusicBrainzAPIError, self).__init__(msg) + super(MusicBrainzAPIError, self).__init__(reason, verb, tb) - def log(self): - """Produce a human-readable log message.""" - return u'MusicBrainz API error: {0}'.format(self) + def get_message(self): + return u'"{0}" in {1} with query {2}'.format( + self._reasonstr(), self.verb, repr(self.query) + ) log = logging.getLogger('beets') @@ -197,7 +196,8 @@ def match_album(artist, album, tracks=None, limit=SEARCH_LIMIT): try: res = _mb_release_search(limit=limit, **criteria) except musicbrainzngs.MusicBrainzError as exc: - raise MusicBrainzAPIError(exc, 'release search', criteria) + raise MusicBrainzAPIError(exc, 'release search', criteria, + traceback.format_exc()) for release in res['release-list']: # The search result is missing some data (namely, the tracks), # so we just use the ID and fetch the rest of the information. @@ -220,7 +220,8 @@ def match_track(artist, title, limit=SEARCH_LIMIT): try: res = _mb_recording_search(limit=limit, **criteria) except musicbrainzngs.MusicBrainzError as exc: - raise MusicBrainzAPIError(exc, 'recording search', criteria) + raise MusicBrainzAPIError(exc, 'recording search', criteria, + traceback.format_exc()) for recording in res['recording-list']: yield track_info(recording) @@ -235,7 +236,8 @@ def album_for_id(albumid): log.debug('Album ID match failed.') return None except musicbrainzngs.MusicBrainzError as exc: - raise MusicBrainzAPIError(exc, 'get release by ID', albumid) + raise MusicBrainzAPIError(exc, 'get release by ID', albumid, + traceback.format_exc()) return album_info(res['release']) def track_for_id(trackid): @@ -248,5 +250,6 @@ def track_for_id(trackid): log.debug('Track ID match failed.') return None except musicbrainzngs.MusicBrainzError as exc: - raise MusicBrainzAPIError(exc, 'get recording by ID', trackid) + raise MusicBrainzAPIError(exc, 'get recording by ID', trackid, + traceback.format_exc()) return track_info(res['recording']) diff --git a/beets/ui/__init__.py b/beets/ui/__init__.py index 443b40698..2059ce5b9 100644 --- a/beets/ui/__init__.py +++ b/beets/ui/__init__.py @@ -749,8 +749,8 @@ def main(args=None, configfh=None): except UserError as exc: message = exc.args[0] if exc.args else None subcommand.parser.error(message) - except util.FilesystemError as exc: - log.error(u'file manipulation error: {0}'.format(exc.args[0])) + except util.HumanReadableException as exc: + exc.log(log) sys.exit(1) except IOError as exc: if exc.errno == errno.EPIPE: diff --git a/beets/util/__init__.py b/beets/util/__init__.py index bddf1374f..49df5684e 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -21,41 +21,88 @@ import re import shutil import fnmatch from collections import defaultdict +import traceback MAX_FILENAME_LENGTH = 200 -class FilesystemError(Exception): - """An error that occurred while performing a filesystem manipulation - via a function in this module. Has three fields: `reason`, the - underlying exception or a string describing the problem; `verb`, the - action being performed; and `paths`, a list of pathnames involved. +class HumanReadableException(Exception): + """An Exception that can include a human-readable error message to + be logged without a traceback. Can preserve a traceback for + debugging purposes as well. + + Has at least two fields: `reason`, the underlying exception or a + string describing the problem; and `verb`, the action being + performed during the error. + + If `tb` is provided, it is a string containing a traceback for the + associated exception. (Note that this is not necessary in Python 3.x + and should be removed when we make the transition.) """ - def __init__(self, reason, verb, paths): + error_kind = 'Error' # Human-readable description of error type. + + def __init__(self, reason, verb, tb=None): self.reason = reason self.verb = verb - self.paths = paths + self.tb = tb + super(HumanReadableException, self).__init__(self.get_message()) - # Use a nicer English phrasing for some specific verbs. - gerund = verb[:-1] if verb.endswith('e') else verb + def _gerund(self): + """Generate a (likely) gerund form of the English verb. + """ + if ' ' in self.verb: + return self.verb + gerund = self.verb[:-1] if self.verb.endswith('e') else self.verb gerund += 'ing' - if verb in ('move', 'copy'): - clause = 'while {0} {1} to {2}'.format(gerund, repr(paths[0]), - repr(paths[1])) - elif verb in ('delete',): - clause = 'while {0} {1}'.format(gerund, repr(paths[0])) + return gerund + + def _reasonstr(self): + """Get the reason as a string.""" + if isinstance(self.reason, basestring): + return self.reason + elif hasattr(self.reason, 'strerror'): # i.e., EnvironmentError + return self.reason.strerror + else: + return u'"{0}"'.format(self.reason) + + def get_message(self): + """Create the human-readable description of the error, sans + introduction. + """ + raise NotImplementedError + + def log(self, logger): + """Log to the provided `logger` a human-readable message as an + error and a verbose traceback as a debug message. + """ + if self.tb: + logger.debug(self.tb) + logger.error(u'{0}: {1}'.format(self.error_kind, self.args[0])) + +class FilesystemError(HumanReadableException): + """An error that occurred while performing a filesystem manipulation + via a function in this module. The `paths` field is a sequence of + pathnames involved in the operation. + """ + def __init__(self, reason, verb, paths, tb=None): + self.paths = paths + super(FilesystemError, self).__init__(reason, verb, tb) + + def get_message(self): + # Use a nicer English phrasing for some specific verbs. + if self.verb in ('move', 'copy'): + clause = 'while {0} {1} to {2}'.format( + self._gerund(), repr(self.paths[0]), repr(self.paths[1]) + ) + elif self.verb in ('delete',): + clause = 'while {0} {1}'.format( + self._gerund(), repr(self.paths[0]) + ) else: clause = 'during {0} of paths {1}'.format( - self.verb, u', '.join(repr(p) for p in paths) + self.verb, u', '.join(repr(p) for p in self.paths) ) - if isinstance(reason, basestring): - reason_str = reason - elif hasattr(reason, 'strerror'): # i.e., EnvironmentError - reason_str = reason.strerror - else: - reason_str = u'"{0}"'.format(reason) - msg = u'{0} {1}'.format(reason_str, clause) - super(FilesystemError, self).__init__(msg) + return u'{0} {1}'.format(self._reasonstr(), clause) def normpath(path): """Provide the canonical form of the path suitable for storing in @@ -264,7 +311,7 @@ def remove(path, soft=True): try: os.remove(path) except (OSError, IOError) as exc: - raise FilesystemError(exc, 'delete', (path,)) + raise FilesystemError(exc, 'delete', (path,), traceback.format_exc()) def copy(path, dest, replace=False, pathmod=os.path): """Copy a plain file. Permissions are not copied. If dest already @@ -281,7 +328,8 @@ def copy(path, dest, replace=False, pathmod=os.path): try: shutil.copyfile(path, dest) except (OSError, IOError) as exc: - raise FilesystemError(exc, 'copy', (path, dest)) + raise FilesystemError(exc, 'copy', (path, dest), + traceback.format_exc()) def move(path, dest, replace=False, pathmod=os.path): """Rename a file. dest may not be a directory. If dest already @@ -293,11 +341,13 @@ def move(path, dest, replace=False, pathmod=os.path): path = syspath(path) dest = syspath(dest) if pathmod.exists(dest): - raise FilesystemError('file exists', 'move', (path, dest)) + raise FilesystemError('file exists', 'move', (path, dest), + traceback.format_exc()) try: shutil.move(path, dest) except (OSError, IOError) as exc: - raise FilesystemError(exc, 'move', (path, dest)) + raise FilesystemError(exc, 'move', (path, dest), + traceback.format_exc()) def unique_path(path): """Returns a version of ``path`` that does not exist on the diff --git a/docs/changelog.rst b/docs/changelog.rst index 232c71ec6..718b5970c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -13,7 +13,8 @@ Changelog halting the importer. * Similarly, filesystem manipulation errors now print helpful error messages instead of a messy traceback. They still interrupt beets, but they should now - be easier for users to understand. + be easier for users to understand. Tracebacks are still available in verbose + mode. * New plugin event: ``import_task_choice`` is called after an import task has an action assigned. * New plugin event: ``library_opened`` is called when beets starts up and