From e7f1ff0e3fa82a8b901d57f7dad51f5429c7e78a Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sun, 10 Aug 2014 16:42:13 -0700 Subject: [PATCH] Clean up `convert --pretend` (#891) There were a number of problems with the changes to the util melange: - It used print rather than logging, and its string formatting was probably not Unicode-ready. - The shell-command-like print lines were not quite compatible, which makes their general usefulness questionable. - Used an unsafe/leaky global variable for mkdirall. - Used deprecated sets.Set. Seemed better just to add this to the plugin where we need it so it's easier to see where this goes. It also seems unnecessary to me to print `mkdir -p` commands. They just clutter up the output for me when I really just want to see the transcoding commands. --- beets/util/__init__.py | 38 +++++--------------------------- beetsplug/convert.py | 49 ++++++++++++++++++++++++++++-------------- docs/changelog.rst | 6 ++++-- 3 files changed, 42 insertions(+), 51 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index e91a12f91..428de312a 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -24,7 +24,6 @@ from collections import defaultdict import traceback import subprocess import platform -from sets import Set MAX_FILENAME_LENGTH = 200 @@ -202,25 +201,10 @@ def sorted_walk(path, ignore=(), logger=None): yield res -# We don't create directories on dry-runs, but we must pretend they exist -directories_created = Set() - - -def mkdirall(path, pretend=False): +def mkdirall(path): """Make all the enclosing directories of path (like mkdir -p on the parent). """ - - if pretend: - # directory = syspath(ancestry(path)[-1]) # "dirname" - # This seems cleaner but MAY have differences on symlinks (leading to - # an equivalent result) - directory = os.path.dirname(path) - if directory not in directories_created: - directories_created.add(directory) - # This is not a "raw" translation, but it's brief one - print("mkdir -p '%s'" % (directory)) - return for ancestor in ancestry(path): if not os.path.isdir(syspath(ancestor)): try: @@ -404,13 +388,10 @@ def samefile(p1, p2): return shutil._samefile(syspath(p1), syspath(p2)) -def remove(path, soft=True, pretend=False): +def remove(path, soft=True): """Remove the file. If `soft`, then no error will be raised if the file does not exist. """ - if pretend: - print("rm '%s'" % (path)) - return path = syspath(path) if soft and not os.path.exists(path): return @@ -420,15 +401,12 @@ def remove(path, soft=True, pretend=False): raise FilesystemError(exc, 'delete', (path,), traceback.format_exc()) -def copy(path, dest, replace=False, pretend=False): +def copy(path, dest, replace=False): """Copy a plain file. Permissions are not copied. If `dest` already exists, raises a FilesystemError unless `replace` is True. Has no effect if `path` is the same as `dest`. Paths are translated to system paths before the syscall. """ - if pretend: - print("cp '%s' '%s'" % (path, dest)) - return if samefile(path, dest): return path = syspath(path) @@ -442,7 +420,7 @@ def copy(path, dest, replace=False, pretend=False): traceback.format_exc()) -def move(path, dest, replace=False, pretend=False): +def move(path, dest, replace=False): """Rename a file. `dest` may not be a directory. If `dest` already exists, raises an OSError unless `replace` is True. Has no effect if `path` is the same as `dest`. If the paths are on different @@ -450,9 +428,6 @@ def move(path, dest, replace=False, pretend=False): instead, in which case metadata will *not* be preserved. Paths are translated to system paths. """ - if pretend: - print("mv '%s' '%s'" % (path, dest)) - return if samefile(path, dest): return path = syspath(path) @@ -643,7 +618,7 @@ def cpu_count(): return 1 -def command_output(cmd, shell=False, pretend=False): +def command_output(cmd, shell=False): """Runs the command and returns its output after it has exited. ``cmd`` is a list of arguments starting with the command names. If @@ -658,9 +633,6 @@ def command_output(cmd, shell=False, pretend=False): Python 2.6 and which can have problems if lots of output is sent to stderr. """ - if pretend: - print(cmd) - return with open(os.devnull, 'wb') as devnull: proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=devnull, close_fds=platform.system() != 'Windows', diff --git a/beetsplug/convert.py b/beetsplug/convert.py index a94f4c284..2b1646b2c 100644 --- a/beetsplug/convert.py +++ b/beetsplug/convert.py @@ -90,7 +90,7 @@ def encode(command, source, dest, pretend=False): quiet = config['convert']['quiet'].get() if not quiet and not pretend: - log.info(u'Started encoding {0}'.format(util.displayable_path(source))) + log.info(u'Encoding {0}'.format(util.displayable_path(source))) command = Template(command).safe_substitute({ 'source': pipes.quote(source), @@ -100,10 +100,12 @@ def encode(command, source, dest, pretend=False): log.debug(u'convert: executing: {0}' .format(util.displayable_path(command))) + if pretend: + log.info(command) + return + try: - util.command_output(command, shell=True, pretend=pretend) - if pretend: - return + util.command_output(command, shell=True) except subprocess.CalledProcessError: # Something went wrong (probably Ctrl+C), remove temporary files log.info(u'Encoding {0} failed. Cleaning up...' @@ -152,8 +154,9 @@ def convert_item(dest_dir, keep_new, path_formats, command, ext, # Ensure that only one thread tries to create directories at a # time. (The existence check is not atomic with the directory # creation inside this function.) - with _fs_lock: - util.mkdirall(dest, pretend) + if not pretend: + with _fs_lock: + util.mkdirall(dest) if os.path.exists(util.syspath(dest)): log.info(u'Skipping {0} (target file exists)'.format( @@ -162,14 +165,29 @@ def convert_item(dest_dir, keep_new, path_formats, command, ext, continue if keep_new: - log.info(u'Moving to {0}'. - format(util.displayable_path(original))) - util.move(item.path, original, pretend) + if pretend: + log.info(u'mv {0} {1}'.format( + util.displayable_path(item.path), + util.displayable_path(original), + )) + else: + log.info(u'Moving to {0}'.format( + util.displayable_path(original)) + ) + util.move(item.path, original) if not should_transcode(item): - # No transcoding necessary. - log.info(u'Copying {0}'.format(util.displayable_path(item.path))) - util.copy(original, converted, pretend) + if pretend: + log.info(u'cp {0} {1}'.format( + util.displayable_path(original), + util.displayable_path(converted), + )) + else: + # No transcoding necessary. + log.info(u'Copying {0}'.format( + util.displayable_path(item.path)) + ) + util.copy(original, converted) else: try: encode(command, original, converted, pretend) @@ -177,8 +195,7 @@ def convert_item(dest_dir, keep_new, path_formats, command, ext, continue if pretend: - # Should we add support for tagging and after_convert plugins? - continue # A yield is used at the start of the loop + continue # Write tags from the database to the converted file. item.write(path=converted) @@ -238,7 +255,7 @@ def convert_func(lib, opts, args): command, ext = get_format(opts.format) pretend = opts.pretend if opts.pretend is not None else \ - config['convert']['pretend'].get() + config['convert']['pretend'].get(bool) if not pretend: ui.commands.list_items(lib, ui.decargs(args), opts.album, None) @@ -299,7 +316,7 @@ class ConvertPlugin(BeetsPlugin): def commands(self): cmd = ui.Subcommand('convert', help='convert to external location') cmd.parser.add_option('-p', '--pretend', action='store_true', - help='only show what would happen') + help='show actions but do nothing') cmd.parser.add_option('-a', '--album', action='store_true', help='choose albums instead of tracks') cmd.parser.add_option('-t', '--threads', action='store', type='int', diff --git a/docs/changelog.rst b/docs/changelog.rst index edc3919c9..01307b6d8 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -67,8 +67,10 @@ Little improvements and fixes: import. * :doc:`/plugins/chroma`: A new ``auto`` configuration option disables fingerprinting on import. Thanks to ddettrittus. -* :doc:`/plugins/convert`: Add ``--format`` option to select the - transoding command from the command-line. +* :doc:`/plugins/convert`: A new ``--format`` option to can select the + transcoding preset from the command-line. +* :doc:`/plugins/convert`: Transcoding presets can now omit their filename + extensions (extensions default to the name of the preset). * A new :ref:`asciify-paths` configuration option replaces all non-ASCII characters in paths.