Selective field updates #2231

dangmai · 2016-10-21T01:13:29Z

PR to fix #2229

dangmai · 2016-10-21T03:35:42Z

beets/ui/commands.py

@@ -1190,6 +1196,10 @@ def update_func(lib, opts, args):
    u'-p', u'--pretend', action='store_true',
    help=u"show all changes but do nothing"
 )
+update_cmd.parser.add_option(
+    u'-F', u'--field', default=None, action='append', dest='fields',


I use -F here instead of -f since the latter is already used by optparse for custom path formatting. If you want it to be something else, I'd be cool with it.

sampsyo

-F seems like a great spelling! Thanks for adding to the docs and tests. ✨

I have a couple of cosmetic suggestions and one substantive one: I'm not sure we need the new parameter to the move method.

sampsyo · 2016-10-21T14:24:51Z

beets/dbcore/db.py

@@ -342,15 +342,17 @@ def __delattr__(self, key):

    # Database interaction (CRUD methods).

-    def store(self):
+    def store(self, fields_to_store=None):
        """Save the object's metadata into the library database.
        """


This is just cosmetic, but I would prefer to use (a) a simpler parameter name like fields and (b) documentation in the docstring here to explain exactly what it does.

sampsyo · 2016-10-21T14:26:28Z

beets/dbcore/db.py

        """Save the object's metadata into the library database.
        """
        self._check_db()

        # Build assignments for query.
        assignments = []
        subvars = []
-        for key in self._fields:
+        if fields_to_store is None:
+            fields_to_store = self._fields


This is again just cosmetic, but I like to put these default-value if blocks to the very top of the function to make it clear what the default values are.

sampsyo · 2016-10-21T14:27:40Z

beets/library.py

-    def store(self):
-        super(LibModel, self).store()
+    def store(self, fields_to_store=None):
+        super(LibModel, self).store(fields_to_store)


(The parameter name should be consistent with the above, if it changes.)

sampsyo · 2016-10-21T14:29:29Z

beets/library.py

@@ -729,7 +729,8 @@ def remove(self, delete=False, with_album=True):

        self._db._memotable = {}

-    def move(self, copy=False, link=False, basedir=None, with_album=True):
+    def move(self, copy=False, link=False, basedir=None, with_album=True,
+             fields_to_store=None):


I'm not sure move needs the new parameter—it's a little unintuitive for clients, and the only reason we store here is to update the paths for files.

sampsyo · 2016-10-21T14:41:38Z

beets/library.py

@@ -1000,7 +1001,7 @@ def move_art(self, copy=False, link=False):
            util.prune_dirs(os.path.dirname(old_art),
                            self._db.directory)

-    def move(self, copy=False, link=False, basedir=None):
+    def move(self, copy=False, link=False, basedir=None, fields_to_store=None):


(As above.)

sampsyo · 2016-10-21T14:42:34Z

beets/library.py

@@ -1108,7 +1110,7 @@ def set_art(self, path, copy=True):

        plugins.send('art_set', album=self)

-    def store(self):
+    def store(self, fields_to_store=None):
        """Update the database with the album information. The album's
        tracks are also updated.
        """


(This probably also needs the name change and docstring expansion.)

sampsyo · 2016-10-21T14:43:28Z

beets/ui/commands.py

@@ -1081,11 +1081,16 @@ def list_func(lib, opts, args):

 # update: Update library contents according to on-disk tags.

-def update_items(lib, query, album, move, pretend):
+def update_items(lib, query, album, move, pretend, fields):
    """For all the items matched by the query, update the library to
    reflect the item's embedded tags.


fields needs a mention in the docstring.

sampsyo · 2016-10-21T14:44:55Z

beets/ui/commands.py

+        if move and fields is not None and 'path' not in fields:
+            # Special case: if an item needs to be moved, the path field has to
+            # updated; otherwise the new path will not be reflected in the
+            # database


Missing a period in this comment.

sampsyo · 2016-10-21T14:47:56Z

beets/ui/commands.py


            # Save changes.
            if not pretend:
                if changed:
                    # Move the item if it's in the library.
                    if move and lib.directory in ancestry(item.path):
-                        item.move()
+                        item.move(fields_to_store=fields)


Because we're about to call item.store, maybe the right thing to do here is just to disable the redundant store to the database inside item.move.

dangmai · 2016-10-21T21:01:07Z

I've pushed a fix for the small cosmetic changes you asked for. The fields param to the move method is a little bit more tricky and I'll (hopefully) tackle it this weekend. My first thought is to specify a store=True argument to move, and if it's False we won't do the automatic storing in that method. Then we can manually handle the storing in the update_items instead. And since it has a default value of True, the other part of beets that depend on that method won't be affected.

Thoughts?

sampsyo · 2016-10-23T20:36:26Z

Fantastic; thank you! I think the store=True argument to move makes the changes there much simpler. I'll merge this now.

Nice work! ✨

Selective field updates

dangmai added 6 commits October 20, 2016 00:04

First attempt for selective field updates

acd2b0e

Fix some test failures

679918f

Fix update test failures

f17601e

Add tests and make sure they pass

406f3ce

Fix long line for flake8

f42f558

Documentation for new flag

8d07bfe

dangmai commented Oct 21, 2016

View reviewed changes

sampsyo requested changes Oct 21, 2016

View reviewed changes

Cosmetic refactors based on feedback

f91362a

dangmai added 2 commits October 23, 2016 15:52

Stop passing in fields to be stored for Item.move and Album.move

04560bd

Fixing line too long

05377ee

sampsyo approved these changes Oct 23, 2016

View reviewed changes

sampsyo merged commit 05377ee into beetbox:master Oct 23, 2016

sampsyo added a commit that referenced this pull request Oct 23, 2016

Merge pull request #2231 from dangmai/selective-update

dab5404

Selective field updates

sampsyo added a commit that referenced this pull request Oct 23, 2016

Changelog/thanks for #2231

c426b76

dangmai deleted the selective-update branch October 23, 2016 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selective field updates #2231

Selective field updates #2231

dangmai commented Oct 21, 2016

dangmai Oct 21, 2016

sampsyo left a comment

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

sampsyo Oct 21, 2016

dangmai commented Oct 21, 2016

sampsyo commented Oct 23, 2016

Selective field updates #2231

Selective field updates #2231

Conversation

dangmai commented Oct 21, 2016

Choose a reason for hiding this comment

sampsyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangmai commented Oct 21, 2016

sampsyo commented Oct 23, 2016