PICARD-1354: Adressed code review comments

picard/mbjson.py

@@ -97,33 +97,39 @@ def _decamelcase(text):
 
 
 _REPLACE_MAP = {}
-_EXTRA_ATTRS = ['guest', 'additional', 'minor', 'solo']
+_PREFIX_ATTRS = ['guest', 'additional', 'minor', 'solo']
 _BLANK_SPECIAL_RELTYPES = {'vocal': 'vocals'}
 
 
-def _transform_attribute(attr, credits):
-    if attr in credits:
-        return credits[attr]
+def _transform_attribute(attr, attr_credits):
+    if attr in attr_credits:
+        return attr_credits[attr]
     else:
         return _decamelcase(_REPLACE_MAP.get(attr, attr))
 
 
-def _parse_attributes(attrs, reltype, credits):
-    attrs = [_transform_attribute(a, credits) for a in attrs]
-    prefix = ' '.join([a for a in attrs if a in _EXTRA_ATTRS])
-    attrs = [a for a in attrs if a not in _EXTRA_ATTRS]
-    len_attrs = len(attrs)
-    if len_attrs > 1:
-        attrs = '%s and %s' % (', '.join(attrs[:-1]), attrs[-1:][0])
-    elif len_attrs == 1:
-        attrs = attrs[0]
+def _parse_attributes(attrs, reltype, attr_credits):
+    prefixes = []
+    nouns = []
+    for attr in attrs:
+        attr = _transform_attribute(attr, attr_credits)
+        if attr in _PREFIX_ATTRS:
+            prefixes.append(attr)
+        else:
+            nouns.append(attr)
+    prefix = ' '.join(prefixes)
+    if len(nouns) > 1:
+        result = '%s and %s' % (', '.join(nouns[:-1]), nouns[-1:][0])
+    elif len(nouns) == 1:
+        result = nouns[0]
     else:
-        attrs = _BLANK_SPECIAL_RELTYPES.get(reltype, '')
-    return ' '.join([prefix, attrs]).strip().lower()
+        result = _BLANK_SPECIAL_RELTYPES.get(reltype, '')
+    return ' '.join([prefix, result]).strip().lower()
 
 
 def _relations_to_metadata(relations, m):
-    use_credited_as = not config.setting["standardize_artists"]
+    use_credited_as = not config.setting['standardize_artists']
+    use_instrument_credits = not config.setting['standardize_instruments']
     for relation in relations:
         if relation['target-type'] == 'artist':
             artist = relation['artist']
@@ -138,11 +144,11 @@ def _relations_to_metadata(relations, m):
             if 'attributes' in relation:
                 attribs = [a for a in relation['attributes']]
             if reltype in ('vocal', 'instrument', 'performer'):
-                if config.setting['use_instrument_credits']:
-                    credits = relation.get('attribute-credits', {})
+                if use_instrument_credits:
+                    attr_credits = relation.get('attribute-credits', {})
                 else:
-                    credits = {}
-                name = 'performer:' + _parse_attributes(attribs, reltype, credits)
+                    attr_credits = {}
+                name = 'performer:' + _parse_attributes(attribs, reltype, attr_credits)
             elif reltype == 'mix-DJ' and len(attribs) > 0:
                 if not hasattr(m, "_djmix_ars"):
                     m._djmix_ars = {}

picard/ui/options/metadata.py

@@ -45,7 +45,7 @@ class MetadataOptionsPage(OptionsPage):
         config.BoolOption("setting", "folksonomy_tags", False),
         config.BoolOption("setting", "convert_punctuation", True),
         config.BoolOption("setting", "standardize_artists", False),
-        config.BoolOption("setting", "use_instrument_credits", False),
+        config.BoolOption("setting", "standardize_instruments", True),
     ]
 
     def __init__(self, parent=None):
@@ -75,7 +75,7 @@ class MetadataOptionsPage(OptionsPage):
         self.ui.va_name.setText(config.setting["va_name"])
         self.ui.nat_name.setText(config.setting["nat_name"])
         self.ui.standardize_artists.setChecked(config.setting["standardize_artists"])
-        self.ui.use_instrument_credits.setChecked(config.setting["use_instrument_credits"])
+        self.ui.standardize_instruments.setChecked(config.setting["standardize_instruments"])
 
     def save(self):
         config.setting["translate_artist_names"] = self.ui.translate_artist_names.isChecked()
@@ -91,7 +91,7 @@ class MetadataOptionsPage(OptionsPage):
             if self.tagger.nats is not None:
                 self.tagger.nats.update()
         config.setting["standardize_artists"] = self.ui.standardize_artists.isChecked()
-        config.setting["use_instrument_credits"] = self.ui.use_instrument_credits.isChecked()
+        config.setting["standardize_instruments"] = self.ui.standardize_instruments.isChecked()
 
     def set_va_name_default(self):
         self.ui.va_name.setText(self.options[0].default)

picard/ui/ui_options_metadata.py

@@ -31,9 +31,9 @@ class Ui_MetadataOptionsPage(object):
         self.standardize_artists = QtWidgets.QCheckBox(self.metadata_groupbox)
         self.standardize_artists.setObjectName("standardize_artists")
         self.verticalLayout_3.addWidget(self.standardize_artists)
-        self.use_instrument_credits = QtWidgets.QCheckBox(self.metadata_groupbox)
-        self.use_instrument_credits.setObjectName("use_instrument_credits")
-        self.verticalLayout_3.addWidget(self.use_instrument_credits)
+        self.standardize_instruments = QtWidgets.QCheckBox(self.metadata_groupbox)
+        self.standardize_instruments.setObjectName("standardize_instruments")
+        self.verticalLayout_3.addWidget(self.standardize_instruments)
         self.convert_punctuation = QtWidgets.QCheckBox(self.metadata_groupbox)
         self.convert_punctuation.setObjectName("convert_punctuation")
         self.verticalLayout_3.addWidget(self.convert_punctuation)
@@ -100,7 +100,7 @@ class Ui_MetadataOptionsPage(object):
         self.metadata_groupbox.setTitle(_("Metadata"))
         self.translate_artist_names.setText(_("Translate artist names to this locale where possible:"))
         self.standardize_artists.setText(_("Use standardized artist names"))
-        self.use_instrument_credits.setText(_("Use instrument and vocal credits"))
+        self.standardize_instruments.setText(_("Use standardized instrument and vocal credits"))
         self.convert_punctuation.setText(_("Convert Unicode punctuation characters to ASCII"))
         self.release_ars.setText(_("Use release relationships"))
         self.track_ars.setText(_("Use track relationships"))
@@ -110,4 +110,3 @@ class Ui_MetadataOptionsPage(object):
         self.label_7.setText(_("Non-album tracks:"))
         self.nat_name_default.setText(_("Default"))
         self.va_name_default.setText(_("Default"))
-

test/test_mbjson.py

@@ -24,7 +24,7 @@ settings = {
     "standardize_artists": False,
     "standardize_releases": False,
     "translate_artist_names": True,
-    "use_instrument_credits": False,
+    "standardize_instruments": True,
     "artist_locale": 'en'
 }
 
@@ -93,7 +93,7 @@ class RecordingTest(MBJSONTest):
 
     def test_recording(self):
         m = Metadata()
-        t = Track("1")
+        t = Track('1')
         recording_to_metadata(self.json_doc, m, t)
         self.assertEqual(m['artist'], 'Ed Sheeran')
         self.assertEqual(m['artists'], 'Ed Sheeran')
@@ -115,8 +115,8 @@ class RecordingTest(MBJSONTest):
 
     def test_recording_instrument_credits(self):
         m = Metadata()
-        t = Track("1")
-        config.setting["use_instrument_credits"] = True
+        t = Track('1')
+        config.setting['standardize_instruments'] = False
         recording_to_metadata(self.json_doc, m, t)
         self.assertEqual(m['performer:vocals'], 'Ed Sheeran')
         self.assertEqual(m['performer:acoustic guitar'], 'Ed Sheeran')

ui/options_metadata.ui

@@ -50,9 +50,9 @@
        </widget>
       </item>
       <item>
-       <widget class="QCheckBox" name="use_instrument_credits">
+       <widget class="QCheckBox" name="standardize_instruments">
         <property name="text">
-         <string>Use instrument and vocal credits</string>
+         <string>Use standardized instrument and vocal credits</string>
         </property>
        </widget>
       </item>