Merge pull request #2687 from zigarrre/issue2682

Refactored move functions for clarity according to #2682

beets/importer.py

@@ -37,7 +37,7 @@
 from beets import plugins
 from beets import util
 from beets import config
-from beets.util import pipeline, sorted_walk, ancestry
+from beets.util import pipeline, sorted_walk, ancestry, MoveOperation
 from beets.util import syspath, normpath, displayable_path
 from enum import Enum
 from beets import mediafile
@@ -675,28 +675,36 @@ def align_album_level_fields(self):
         for item in self.items:
             item.update(changes)
 
-    def manipulate_files(self, move=False, copy=False, write=False,
-                         link=False, hardlink=False, session=None):
+    def manipulate_files(self, operation=None, write=False, session=None):
+        """ Copy, move, link or hardlink (depending on `operation`) the files
+        as well as write metadata.
+
+        `operation` should be an instance of `util.MoveOperation`.
+
+        If `write` is `True` metadata is written to the files.
+        """
+
         items = self.imported_items()
         # Save the original paths of all items for deletion and pruning
         # in the next step (finalization).
         self.old_paths = [item.path for item in items]
         for item in items:
-            if move or copy or link or hardlink:
+            if operation is not None:
                 # In copy and link modes, treat re-imports specially:
                 # move in-library files. (Out-of-library files are
                 # copied/moved as usual).
                 old_path = item.path
-                if (copy or link or hardlink) and self.replaced_items[item] \
-                   and session.lib.directory in util.ancestry(old_path):
+                if (operation != MoveOperation.MOVE
+                        and self.replaced_items[item]
+                        and session.lib.directory in util.ancestry(old_path)):
                     item.move()
                     # We moved the item, so remove the
                     # now-nonexistent file from old_paths.
                     self.old_paths.remove(old_path)
                 else:
                     # A normal import. Just copy files and keep track of
                     # old paths.
-                    item.move(copy, link, hardlink)
+                    item.move(operation)
 
             if write and (self.apply or self.choice_flag == action.RETAG):
                 item.try_write()
@@ -1450,12 +1458,20 @@ def manipulate_files(session, task):
         if task.should_remove_duplicates:
             task.remove_duplicates(session.lib)
 
+        if session.config['move']:
+            operation = MoveOperation.MOVE
+        elif session.config['copy']:
+            operation = MoveOperation.COPY
+        elif session.config['link']:
+            operation = MoveOperation.LINK
+        elif session.config['hardlink']:
+            operation = MoveOperation.HARDLINK
+        else:
+            operation = None
+
         task.manipulate_files(
-            move=session.config['move'],
-            copy=session.config['copy'],
+            operation,
             write=session.config['write'],
-            link=session.config['link'],
-            hardlink=session.config['hardlink'],
             session=session,
         )
 

beets/library.py

@@ -28,7 +28,8 @@
 from beets.mediafile import MediaFile, UnreadableFileError
 from beets import plugins
 from beets import util
-from beets.util import bytestring_path, syspath, normpath, samefile
+from beets.util import bytestring_path, syspath, normpath, samefile, \
+    MoveOperation
 from beets.util.functemplate import Template
 from beets import dbcore
 from beets.dbcore import types
@@ -685,31 +686,34 @@ def try_sync(self, write, move, with_album=True):
 
     # Files themselves.
 
-    def move_file(self, dest, copy=False, link=False, hardlink=False):
-        """Moves or copies the item's file, updating the path value if
-        the move succeeds. If a file exists at ``dest``, then it is
-        slightly modified to be unique.
+    def move_file(self, dest, operation=MoveOperation.MOVE):
+        """Move, copy, link or hardlink the item's depending on `operation`,
+        updating the path value if the move succeeds.
+
+        If a file exists at `dest`, then it is slightly modified to be unique.
+
+        `operation` should be an instance of `util.MoveOperation`.
         """
         if not util.samefile(self.path, dest):
             dest = util.unique_path(dest)
-        if copy:
+        if operation == MoveOperation.MOVE:
+            plugins.send("before_item_moved", item=self, source=self.path,
+                         destination=dest)
+            util.move(self.path, dest)
+            plugins.send("item_moved", item=self, source=self.path,
+                         destination=dest)
+        elif operation == MoveOperation.COPY:
             util.copy(self.path, dest)
             plugins.send("item_copied", item=self, source=self.path,
                          destination=dest)
-        elif link:
+        elif operation == MoveOperation.LINK:
             util.link(self.path, dest)
             plugins.send("item_linked", item=self, source=self.path,
                          destination=dest)
-        elif hardlink:
+        elif operation == MoveOperation.HARDLINK:
             util.hardlink(self.path, dest)
             plugins.send("item_hardlinked", item=self, source=self.path,
                          destination=dest)
-        else:
-            plugins.send("before_item_moved", item=self, source=self.path,
-                         destination=dest)
-            util.move(self.path, dest)
-            plugins.send("item_moved", item=self, source=self.path,
-                         destination=dest)
 
         # Either copying or moving succeeded, so update the stored path.
         self.path = dest
@@ -756,29 +760,27 @@ def remove(self, delete=False, with_album=True):
 
         self._db._memotable = {}
 
-    def move(self, copy=False, link=False, hardlink=False, basedir=None,
+    def move(self, operation=MoveOperation.MOVE, basedir=None,
              with_album=True, store=True):
         """Move the item to its designated location within the library
         directory (provided by destination()). Subdirectories are
         created as needed. If the operation succeeds, the item's path
         field is updated to reflect the new location.
 
-        If `copy` is true, moving the file is copied rather than moved.
-        Similarly, `link` creates a symlink instead, and `hardlink`
-        creates a hardlink.
+        Instead of moving the item it can also be copied, linked or hardlinked
+        depending on `operation` which should be an instance of
+        `util.MoveOperation`.
 
-        basedir overrides the library base directory for the
-        destination.
+        `basedir` overrides the library base directory for the destination.
 
-        If the item is in an album, the album is given an opportunity to
-        move its art. (This can be disabled by passing
-        with_album=False.)
+        If the item is in an album and `with_album` is `True`, the album is
+        given an opportunity to move its art.
 
         By default, the item is stored to the database if it is in the
         database, so any dirty fields prior to the move() call will be written
-        as a side effect. You probably want to call save() to commit the DB
-        transaction. If `store` is true however, the item won't be stored, and
-        you'll have to manually store it after invoking this method.
+        as a side effect.
+        If `store` is `False` however, the item won't be stored and you'll
+        have to manually store it after invoking this method.
         """
         self._check_db()
         dest = self.destination(basedir=basedir)
@@ -788,20 +790,20 @@ def move(self, copy=False, link=False, hardlink=False, basedir=None,
 
         # Perform the move and store the change.
         old_path = self.path
-        self.move_file(dest, copy, link, hardlink)
+        self.move_file(dest, operation)
         if store:
             self.store()
 
         # If this item is in an album, move its art.
         if with_album:
             album = self.get_album()
             if album:
-                album.move_art(copy)
+                album.move_art(operation)
                 if store:
                     album.store()
 
         # Prune vacated directory.
-        if not copy:
+        if operation == MoveOperation.MOVE:
             util.prune_dirs(os.path.dirname(old_path), self._db.directory)
 
     # Templating.
@@ -1008,9 +1010,12 @@ def remove(self, delete=False, with_items=True):
             for item in self.items():
                 item.remove(delete, False)
 
-    def move_art(self, copy=False, link=False, hardlink=False):
-        """Move or copy any existing album art so that it remains in the
-        same directory as the items.
+    def move_art(self, operation=MoveOperation.MOVE):
+        """Move, copy, link or hardlink (depending on `operation`) any
+        existing album art so that it remains in the same directory as
+        the items.
+
+        `operation` should be an instance of `util.MoveOperation`.
         """
         old_art = self.artpath
         if not old_art:
@@ -1024,29 +1029,29 @@ def move_art(self, copy=False, link=False, hardlink=False):
         log.debug(u'moving album art {0} to {1}',
                   util.displayable_path(old_art),
                   util.displayable_path(new_art))
-        if copy:
+        if operation == MoveOperation.MOVE:
+            util.move(old_art, new_art)
+            util.prune_dirs(os.path.dirname(old_art), self._db.directory)
+        elif operation == MoveOperation.COPY:
             util.copy(old_art, new_art)
-        elif link:
+        elif operation == MoveOperation.LINK:
             util.link(old_art, new_art)
-        elif hardlink:
+        elif operation == MoveOperation.HARDLINK:
             util.hardlink(old_art, new_art)
-        else:
-            util.move(old_art, new_art)
         self.artpath = new_art
 
-        # Prune old path when moving.
-        if not copy:
-            util.prune_dirs(os.path.dirname(old_art),
-                            self._db.directory)
-
-    def move(self, copy=False, link=False, hardlink=False, basedir=None,
-             store=True):
-        """Moves (or copies) all items to their destination. Any album
-        art moves along with them. basedir overrides the library base
-        directory for the destination. By default, the album is stored to the
-        database, persisting any modifications to its metadata. If `store` is
-        true however, the album is not stored automatically, and you'll have
-        to manually store it after invoking this method.
+    def move(self, operation=MoveOperation.MOVE, basedir=None, store=True):
+        """Move, copy, link or hardlink (depending on `operation`)
+        all items to their destination. Any album art moves along with them.
+
+        `basedir` overrides the library base directory for the destination.
+
+        `operation` should be an instance of `util.MoveOperation`.
+
+        By default, the album is stored to the database, persisting any
+        modifications to its metadata. If `store` is `False` however,
+        the album is not stored automatically, and you'll have to manually
+        store it after invoking this method.
         """
         basedir = basedir or self._db.directory
 
@@ -1058,11 +1063,11 @@ def move(self, copy=False, link=False, hardlink=False, basedir=None,
         # Move items.
         items = list(self.items())
         for item in items:
-            item.move(copy, link, hardlink, basedir=basedir, with_album=False,
+            item.move(operation, basedir=basedir, with_album=False,
                       store=store)
 
         # Move art.
-        self.move_art(copy, link, hardlink)
+        self.move_art(operation)
         if store:
             self.store()
 

beets/ui/commands.py

@@ -34,7 +34,8 @@
 from beets import plugins
 from beets import importer
 from beets import util
-from beets.util import syspath, normpath, ancestry, displayable_path
+from beets.util import syspath, normpath, ancestry, displayable_path, \
+    MoveOperation
 from beets import library
 from beets import config
 from beets import logging
@@ -1499,10 +1500,14 @@ def move_items(lib, dest, query, copy, album, pretend, confirm=False,
 
             if export:
                 # Copy without affecting the database.
-                obj.move(True, basedir=dest, store=False)
+                obj.move(operation=MoveOperation.COPY, basedir=dest,
+                         store=False)
             else:
                 # Ordinary move/copy: store the new path.
-                obj.move(copy, basedir=dest)
+                if copy:
+                    obj.move(operation=MoveOperation.COPY, basedir=dest)
+                else:
+                    obj.move(operation=MoveOperation.MOVE, basedir=dest)
 
 
 def move_func(lib, opts, args):

beets/util/__init__.py

@@ -31,6 +31,7 @@
 from beets.util import hidden
 import six
 from unidecode import unidecode
+from enum import Enum
 
 
 MAX_FILENAME_LENGTH = 200
@@ -124,6 +125,15 @@ def get_message(self):
         return u'{0} {1}'.format(self._reasonstr(), clause)
 
 
+class MoveOperation(Enum):
+    """The file operations that e.g. various move functions can carry out.
+    """
+    MOVE = 0
+    COPY = 1
+    LINK = 2
+    HARDLINK = 3
+
+
 def normpath(path):
     """Provide the canonical form of the path suitable for storing in
     the database.

beetsplug/duplicates.py

@@ -22,7 +22,7 @@
 from beets.plugins import BeetsPlugin
 from beets.ui import decargs, print_, Subcommand, UserError
 from beets.util import command_output, displayable_path, subprocess, \
-    bytestring_path
+    bytestring_path, MoveOperation
 from beets.library import Item, Album
 import six
 
@@ -175,10 +175,10 @@ def _process_item(self, item, copy=False, move=False, delete=False,
         """
         print_(format(item, fmt))
         if copy:
-            item.move(basedir=copy, copy=True)
+            item.move(basedir=copy, operation=MoveOperation.COPY)
             item.store()
         if move:
-            item.move(basedir=move, copy=False)
+            item.move(basedir=move)
             item.store()
         if delete:
             item.remove(delete=True)
@@ -312,7 +312,7 @@ def _merge_albums(self, objs):
                                     objs[0],
                                     displayable_path(o.path),
                                     displayable_path(missing.destination()))
-                    missing.move(copy=True)
+                    missing.move(operation=MoveOperation.COPY)
         return objs
 
     def _merge(self, objs):

docs/changelog.rst

@@ -50,6 +50,9 @@ For developers:
   missing values into type-specific null-like values. This should help in
   cases where a string field is unexpectedly `None` sometimes instead of just
   showing up as an empty string. :bug:`2605`
+* Refactored the move functions in library.py and the `manipulate_files` function
+  in importer.py to use a single parameter describing the file operation instead
+  of multiple boolean flags. :bug:`2682`
 
 
 1.4.5 (June 20, 2017)

setup.cfg

@@ -5,8 +5,8 @@ logging-clear-handlers=1
 [flake8]
 min-version=2.7
 accept-encodings=utf-8
-# Default pyflakes errors we ignore:
-# - E241: missing whitespace after ',' (used to align visually)
+# Errors we ignore:
+# - E121,E123,E126,E226,E24,E704,W503,W504 flake8 default ignores (have to be listed here to not be overridden)
 # - E221: multiple spaces before operator (used to align visually)
 # - E731: do not assign a lambda expression, use a def
 # - F405 object may be undefined, or defined from star imports
@@ -19,4 +19,4 @@ accept-encodings=utf-8
 # - FI53: `__future__` import "print_function" present
 # - FI14: `__future__` import "unicode_literals" missing
 # - FI15: `__future__` import "generator_stop" missing
-ignore=E305,C901,E241,E221,E731,F405,FI50,FI51,FI12,FI53,FI14,FI15
+ignore=E121,E123,E126,E226,E24,E704,W503,W504,E305,C901,E221,E731,F405,FI50,FI51,FI12,FI53,FI14,FI15