Set field values on the import command line #1881 #2581

bartkl · 2017-06-01T10:10:55Z

My first contribution, so be gentle ;).

I've tested for both album and singleton import modes, using both as-is and autotagged import, and it seems to work fine.
I extended the test-suite of the importer with test_set_fields functions in the SingletonImportTest and ImportTest classes.
I added documentation in cli.rst and config.rst, and updated the changelog.
tox seems to approve of my style and the tests (including mine) pass

I can imagine the _store_dict custom callback action I created should be moved elsewhere, but I do think this generic approach is better than a more helper function for the importer_cmd specifically.

I'd love to hear your comments!

…lds functionality in importer

mention-bot · 2017-06-01T10:11:03Z

@bartkl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mattmurch, @geigerzaehler and @sampsyo to be potential reviewers.

bartkl · 2017-06-07T12:27:18Z

beets/config_default.yaml

@@ -25,7 +25,8 @@ import:
    pretend: false
    search_ids: []
    duplicate_action: ask
-    bell: no  
+    bell: no


removed a redundant space after 'bell: no'

sampsyo

Thanks for your patience while I found the time to do a full review! This looks fantastic already; thank you for the careful and complete implementation. ✨ I've left a few suggestions scattered throughout the code.

sampsyo · 2017-06-11T16:50:34Z

beets/importer.py

@@ -530,6 +533,17 @@ def remove_duplicates(self, lib):
                util.prune_dirs(os.path.dirname(item.path),
                                lib.directory)

+    def set_fields(self):


A docstring here might be nice to have! (Perhaps even nicer than the summary in the overall "step sequence" in the overall class docstring.)

sampsyo · 2017-06-11T16:51:59Z

beets/importer.py

@@ -530,6 +533,17 @@ def remove_duplicates(self, lib):
                util.prune_dirs(os.path.dirname(item.path),
                                lib.directory)

+    def set_fields(self):
+        set_fields_dict = config['import']['set_fields']
+        for field in set_fields_dict.keys():


A simpler way to iterate over Confit dicts:

for key, view in config['import']['set_fields'].items(): value = view.get()

Thanks for the tip: done.

sampsyo · 2017-06-11T16:52:45Z

beets/importer.py

+                      field,
+                      value)
+            self.album[field] = value
+            self.album.store()


I recommend not including a store call here. Instead, we can just modify the values before they ever hit the database at all! Another comment below will elaborate…

Scratch my previous comment in light of the ordering constraints, but I still recommend "un-indenting" this line so the album get stored only once to update all of its fields. (And the same below for items.)

Ah of course, way better indeed :), will do.

sampsyo · 2017-06-11T16:53:42Z

beets/importer.py

+    # If ``set_fields`` is set, set those fields to the
+    # configured values.
+    if config['import']['set_fields']:
+        task.set_fields()


I recommend moving this stanza above the call to task.add. This way, we'll modify the values on the Album before ever putting them in the database. This will be marginally more efficient and, more importantly, will be a little more predictable.

I actually used to have it structured that way, but it doesn't work. Without the call to task.add first, there's no album attribute defined on the task: AttributeError: 'ImportTask' object has no attribute 'album'.

Please let me know it you have another idea, because I do agree with you it would be better.

Aha; got it! That makes sense. Let's leave it where it is—perhaps with a comment to clarify why it's there?—for this version at least.

I'll add a note.

sampsyo · 2017-06-11T16:53:59Z

beets/importer.py

+                      field,
+                      value)
+            self.item[field] = value
+            self.item.store()


(As above.)

sampsyo · 2017-06-11T16:55:16Z

beets/ui/commands.py

@@ -1017,6 +1018,12 @@ def import_func(lib, opts, args):
    metavar='ID',
    help=u'restrict matching to a specific metadata backend ID'
 )
+import_cmd.parser.add_option(
+    u'--set-field', dest='set_fields', action='callback',


This is a little bit of bikeshedding, but what do you think about the slightly simpler option name --set? In beets, the only thing you can really "set" is fields, so having "field" in the name feels a teensy bit redundant.

Agreed, and there's the metavar value which makes it sufficiently explicit. I'll change this.

sampsyo · 2017-06-11T16:56:37Z

beets/ui/__init__.py

@@ -1060,6 +1060,32 @@ def parse_subcommand(self, args):
        suboptions, subargs = subcommand.parse_args(args)
        return subcommand, suboptions, subargs

+    @staticmethod
+    def _store_dict(option, opt_str, value, parser):


This is a nice way to parse a series of foo=bar options! Cool!

But I don't think this needs to be a static method of the SubcommandsOptionParser class, which is intended for a pretty specific purpose. Can we make it a plain, top-level function?

Had my doubts on where to place this. I'll follow your advice :).

sampsyo · 2017-06-11T16:57:24Z

docs/reference/cli.rst

+  those fields the specified values on import, in addition to such field/value
+  pairs defined in the ``importer.set_fields`` dictionary in the configuration
+  file. Make sure to use an option per field/value pair, like so:
+  ::


In ReST, if you just use :: at the end of your paragraph, you'll get a literal : there and mark the following indented block as code.

Thanks for the tip, I improved it.

bartkl · 2017-06-12T14:41:27Z

Thanks for your feedback and kind words :).
I've corrected all the changes you've requested except for the ones involving removing the store() calls and moving the set_fields logic above the task.add call for reasons explained above. I'll commit the changes done so far, and I'll hear from you if it needs any extra work.

bartkl · 2017-06-12T15:12:20Z

I hope I'm not too much of a burden, but I don't get what goes wrong with the py27-flake8 and python 3 tests. Could you help me out there?

And the merge conflict is probably not something I can or should resolve, right?

Thanks!

sampsyo · 2017-06-12T22:08:23Z

Sure. Here are the results of the flake8 run (which you can get by typing tox -e py27-flake8 or just installing flake8 and running flake8 beets beetsplug test to check everything):

beets/ui/__init__.py:774:1: E302 expected 2 blank lines, found 1
test/test_importer.py:561:24: E261 at least two spaces before inline comment
test/test_importer.py:722:25: E261 at least two spaces before inline comment

Those are some fairly banal style complaints: just about whitespace. In particular, you'll want to do this:

print()  # Foo.

instead of:

print() # Foo.

Tiny detail, I know!

I'm not seeing any Python 3 test failures on my end—is there a specific message you're looking at?

About merge conflicts: it's actually fairly straightforward for you to resolve this on your end. You can just type git merge master to bring everything up to date. You'll need to manually resolve the conflicts, but I think that will just be the changelog (sorry about that! Creating a new release meant monkeying with the changelog a bit).

bartkl · 2017-06-13T12:57:42Z

It seems to be fine :).

sampsyo · 2017-06-13T13:28:29Z

Awesome!! Thank you for your hard work on this; I look forward to using this feature myself. Merging now…

Set field values on the import command line #1881

bartkl added 5 commits May 30, 2017 16:15

implemented set_field cli parsing

1da972f

store_dict parser action now stores unicode, and implementend set_fie…

569098a

…lds functionality in importer

added documentation

91722ae

implemented error handling for invalid, non-key/value cli argumnets

b5a8891

finished tests for set_fields on importer

c648781

bartkl commented Jun 7, 2017

View reviewed changes

sampsyo requested changes Jun 11, 2017

View reviewed changes

bartkl added 3 commits June 12, 2017 16:46

refactoring according to feedback in pull request

52d5d23

added docstring for set_fields methods

53d0421

refactoring according to suggestions in pull request

2465898

bartkl added 2 commits June 13, 2017 14:33

revised to fix flake8 warnings

762f9ca

Merge branch 'master' into master

2eb4e3d

sampsyo merged commit 2eb4e3d into beetbox:master Jun 13, 2017

sampsyo added a commit that referenced this pull request Jun 13, 2017

Merge pull request #2581 from bartkl/master

e0ce507

Set field values on the import command line #1881

sampsyo added a commit that referenced this pull request Jun 13, 2017

Docs refinements for #2581

71d6dc3

sampsyo mentioned this pull request Jun 13, 2017

Set field values on the import command line #1881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set field values on the import command line #1881 #2581

Set field values on the import command line #1881 #2581

bartkl commented Jun 1, 2017

mention-bot commented Jun 1, 2017

bartkl Jun 7, 2017

sampsyo left a comment

sampsyo Jun 11, 2017

bartkl Jun 12, 2017

sampsyo Jun 11, 2017

bartkl Jun 13, 2017

sampsyo Jun 11, 2017

sampsyo Jun 12, 2017

bartkl Jun 12, 2017

sampsyo Jun 11, 2017

bartkl Jun 12, 2017

sampsyo Jun 12, 2017

bartkl Jun 12, 2017

sampsyo Jun 11, 2017

sampsyo Jun 11, 2017

bartkl Jun 12, 2017

sampsyo Jun 11, 2017

bartkl Jun 12, 2017

sampsyo Jun 11, 2017

bartkl Jun 12, 2017

bartkl commented Jun 12, 2017

bartkl commented Jun 12, 2017

sampsyo commented Jun 12, 2017

bartkl commented Jun 13, 2017

sampsyo commented Jun 13, 2017

Set field values on the import command line #1881 #2581

Set field values on the import command line #1881 #2581

Conversation

bartkl commented Jun 1, 2017

mention-bot commented Jun 1, 2017

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

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

bartkl commented Jun 12, 2017

bartkl commented Jun 12, 2017

sampsyo commented Jun 12, 2017

bartkl commented Jun 13, 2017

sampsyo commented Jun 13, 2017