From b3761396638f5b0da1e3111dd450d40d0d86a464 Mon Sep 17 00:00:00 2001 From: zigarrre Date: Mon, 11 Sep 2017 16:35:26 +0200 Subject: [PATCH 1/3] Refactored move functions for clarity according to #2682 The move functions in library.py and manipule_files in importer.py where changed to use a single parameter for the file operation instead of multiple boolean flags. A typo in the documentation of the Album.move and Item.move functions confusing True and False when describing the store parameter was fixed as well. --- beets/importer.py | 38 ++++++++++---- beets/library.py | 108 +++++++++++++++++++++------------------- beets/ui/commands.py | 11 ++-- beets/util/__init__.py | 10 ++++ beetsplug/duplicates.py | 8 +-- docs/changelog.rst | 3 ++ test/helper.py | 7 +-- test/test_files.py | 35 ++++++------- test/test_ui.py | 6 +-- 9 files changed, 133 insertions(+), 93 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index d36a5a0a09..e7d8b40bc3 100644 --- a/beets/importer.py +++ b/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,20 +675,28 @@ 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. @@ -696,7 +704,7 @@ def manipulate_files(self, move=False, copy=False, write=False, 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, ) diff --git a/beets/library.py b/beets/library.py index 988628a5c5..dd9081d101 100644 --- a/beets/library.py +++ b/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,26 @@ 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,7 +789,7 @@ 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() @@ -796,12 +797,12 @@ def move(self, copy=False, link=False, hardlink=False, basedir=None, 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 +1009,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 +1028,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 +1062,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() diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 4171ae3b1a..a4b4339255 100644 --- a/beets/ui/commands.py +++ b/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): diff --git a/beets/util/__init__.py b/beets/util/__init__.py index ee06df0bf5..6673c61e34 100644 --- a/beets/util/__init__.py +++ b/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): + """Represents 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. diff --git a/beetsplug/duplicates.py b/beetsplug/duplicates.py index 2f6bba3e6a..f63a7be20c 100644 --- a/beetsplug/duplicates.py +++ b/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): diff --git a/docs/changelog.rst b/docs/changelog.rst index 1c86011fdb..f8b8ad1cd2 100644 --- a/docs/changelog.rst +++ b/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) diff --git a/test/helper.py b/test/helper.py index 5af742e12b..ebf039fca8 100644 --- a/test/helper.py +++ b/test/helper.py @@ -52,6 +52,7 @@ from beets.autotag.hooks import AlbumInfo, TrackInfo from beets.mediafile import MediaFile, Image from beets import util +from beets.util import MoveOperation # TODO Move AutotagMock here from test import _common @@ -349,7 +350,7 @@ def add_item_fixture(self, **values): item['path'] = os.path.join(_common.RSRC, util.bytestring_path('min.' + extension)) item.add(self.lib) - item.move(copy=True) + item.move(operation=MoveOperation.COPY) item.store() return item @@ -370,7 +371,7 @@ def add_item_fixtures(self, ext='mp3', count=1): # mtime needs to be set last since other assignments reset it. item.mtime = 12345 item.add(self.lib) - item.move(copy=True) + item.move(operation=MoveOperation.COPY) item.store() items.append(item) return items @@ -387,7 +388,7 @@ def add_album_fixture(self, track_count=1, ext='mp3'): # mtime needs to be set last since other assignments reset it. item.mtime = 12345 item.add(self.lib) - item.move(copy=True) + item.move(operation=MoveOperation.COPY) item.store() items.append(item) return self.lib.add_album(items) diff --git a/test/test_files.py b/test/test_files.py index 834d3391ce..ff055ac6f2 100644 --- a/test/test_files.py +++ b/test/test_files.py @@ -27,6 +27,7 @@ from test._common import item, touch import beets.library from beets import util +from beets.util import MoveOperation class MoveTest(_common.TestCase): @@ -78,11 +79,11 @@ def test_move_in_lib_prunes_empty_dir(self): self.assertNotExists(os.path.dirname(old_path)) def test_copy_arrives(self): - self.i.move(copy=True) + self.i.move(operation=MoveOperation.COPY) self.assertExists(self.dest) def test_copy_does_not_depart(self): - self.i.move(copy=True) + self.i.move(operation=MoveOperation.COPY) self.assertExists(self.path) def test_move_changes_path(self): @@ -92,13 +93,13 @@ def test_move_changes_path(self): def test_copy_already_at_destination(self): self.i.move() old_path = self.i.path - self.i.move(copy=True) + self.i.move(operation=MoveOperation.COPY) self.assertEqual(self.i.path, old_path) def test_move_already_at_destination(self): self.i.move() old_path = self.i.path - self.i.move(copy=False) + self.i.move() self.assertEqual(self.i.path, old_path) def test_read_only_file_copied_writable(self): @@ -106,7 +107,7 @@ def test_read_only_file_copied_writable(self): os.chmod(self.path, 0o444) try: - self.i.move(copy=True) + self.i.move(operation=MoveOperation.COPY) self.assertTrue(os.access(self.i.path, os.W_OK)) finally: # Make everything writable so it can be cleaned up. @@ -126,24 +127,24 @@ def test_move_avoids_collision_with_existing_file(self): @unittest.skipUnless(_common.HAVE_SYMLINK, "need symlinks") def test_link_arrives(self): - self.i.move(link=True) + self.i.move(operation=MoveOperation.LINK) self.assertExists(self.dest) self.assertTrue(os.path.islink(self.dest)) self.assertEqual(os.readlink(self.dest), self.path) @unittest.skipUnless(_common.HAVE_SYMLINK, "need symlinks") def test_link_does_not_depart(self): - self.i.move(link=True) + self.i.move(operation=MoveOperation.LINK) self.assertExists(self.path) @unittest.skipUnless(_common.HAVE_SYMLINK, "need symlinks") def test_link_changes_path(self): - self.i.move(link=True) + self.i.move(operation=MoveOperation.LINK) self.assertEqual(self.i.path, util.normpath(self.dest)) @unittest.skipUnless(_common.HAVE_HARDLINK, "need hardlinks") def test_hardlink_arrives(self): - self.i.move(hardlink=True) + self.i.move(operation=MoveOperation.HARDLINK) self.assertExists(self.dest) s1 = os.stat(self.path) s2 = os.stat(self.dest) @@ -154,12 +155,12 @@ def test_hardlink_arrives(self): @unittest.skipUnless(_common.HAVE_HARDLINK, "need hardlinks") def test_hardlink_does_not_depart(self): - self.i.move(hardlink=True) + self.i.move(operation=MoveOperation.HARDLINK) self.assertExists(self.path) @unittest.skipUnless(_common.HAVE_HARDLINK, "need hardlinks") def test_hardlink_changes_path(self): - self.i.move(hardlink=True) + self.i.move(operation=MoveOperation.HARDLINK) self.assertEqual(self.i.path, util.normpath(self.dest)) @@ -236,7 +237,7 @@ def test_albuminfo_move_moves_file(self): def test_albuminfo_move_copies_file(self): oldpath = self.i.path self.ai.album = u'newAlbumName' - self.ai.move(True) + self.ai.move(operation=MoveOperation.COPY) self.ai.store() self.i.load() @@ -311,7 +312,7 @@ def test_setart_copies_image(self): i2.path = self.i.path i2.artist = u'someArtist' ai = self.lib.add_album((i2,)) - i2.move(True) + i2.move(operation=MoveOperation.COPY) self.assertEqual(ai.artpath, None) ai.set_art(newart) @@ -327,7 +328,7 @@ def test_setart_to_existing_art_works(self): i2.path = self.i.path i2.artist = u'someArtist' ai = self.lib.add_album((i2,)) - i2.move(True) + i2.move(operation=MoveOperation.COPY) ai.set_art(newart) # Set the art again. @@ -341,7 +342,7 @@ def test_setart_to_existing_but_unset_art_works(self): i2.path = self.i.path i2.artist = u'someArtist' ai = self.lib.add_album((i2,)) - i2.move(True) + i2.move(operation=MoveOperation.COPY) # Copy the art to the destination. artdest = ai.art_destination(newart) @@ -358,7 +359,7 @@ def test_setart_to_conflicting_file_gets_new_path(self): i2.path = self.i.path i2.artist = u'someArtist' ai = self.lib.add_album((i2,)) - i2.move(True) + i2.move(operation=MoveOperation.COPY) # Make a file at the destination. artdest = ai.art_destination(newart) @@ -382,7 +383,7 @@ def test_setart_sets_permissions(self): i2.path = self.i.path i2.artist = u'someArtist' ai = self.lib.add_album((i2,)) - i2.move(True) + i2.move(operation=MoveOperation.COPY) ai.set_art(newart) mode = stat.S_IMODE(os.stat(ai.artpath).st_mode) diff --git a/test/test_ui.py b/test/test_ui.py index 04c033f51a..77804d3d2d 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -40,7 +40,7 @@ from beets import plugins from beets.util.confit import ConfigError from beets import util -from beets.util import syspath +from beets.util import syspath, MoveOperation class ListTest(unittest.TestCase): @@ -126,7 +126,7 @@ def setUp(self): item_path = os.path.join(_common.RSRC, b'full.mp3') self.i = library.Item.from_path(item_path) self.lib.add(self.i) - self.i.move(True) + self.i.move(operation=MoveOperation.COPY) def test_remove_items_no_delete(self): self.io.addinput('y') @@ -509,7 +509,7 @@ def setUp(self): item_path = os.path.join(_common.RSRC, b'full.mp3') self.i = library.Item.from_path(item_path) self.lib.add(self.i) - self.i.move(True) + self.i.move(operation=MoveOperation.COPY) self.album = self.lib.add_album([self.i]) # Album art. From edd2d42cd0cbc10e66567fa8c7bdef62fc770862 Mon Sep 17 00:00:00 2001 From: zigarrre Date: Mon, 11 Sep 2017 17:29:57 +0200 Subject: [PATCH 2/3] Shortened two lines that where to long. --- beets/library.py | 5 +++-- beets/util/__init__.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/beets/library.py b/beets/library.py index dd9081d101..597cfe625b 100644 --- a/beets/library.py +++ b/beets/library.py @@ -778,8 +778,9 @@ def move(self, operation=MoveOperation.MOVE, basedir=None, 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. If `store` is `False` 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) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 6673c61e34..db341a646e 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -126,7 +126,7 @@ def get_message(self): class MoveOperation(Enum): - """Represents the file operations that e.g. various move functions can carry out. + """The file operations that e.g. various move functions can carry out. """ MOVE = 0 COPY = 1 From e30513db9bd2743820f4ccfadd46fb29d0b054e3 Mon Sep 17 00:00:00 2001 From: zigarrre Date: Tue, 12 Sep 2017 18:19:18 +0200 Subject: [PATCH 3/3] Added the default ignore list of flake8 This fixes the W503 warnings and possibly other, unwanted errors and warnings in the future. --- setup.cfg | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/setup.cfg b/setup.cfg index 913880415f..a40d74964b 100644 --- a/setup.cfg +++ b/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