Add composer_sort as a tag #2529

dosoe · 2017-04-28T10:20:04Z

Already discussed on #2519
This is my first pull request (almost) and my first contribution to any open-source project, so please be kind...
It's heavily using https://github.com/beetbox/beets/pull/2333/files/7060b512b848fb36f49dcc091f65a6e7baa83d4f and https://picard.musicbrainz.org/docs/mappings/ . I would have liked to add an arranger_sort tag, but arranger_sort is not listed in the tag mapping (unlike composer_sort), so it will be more difficult.

mention-bot · 2017-04-28T10:20:06Z

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

dosoe · 2017-04-28T10:22:00Z

It works fine on my files, but I didn't do any further testing.

dosoe · 2017-04-28T10:25:42Z

Concerning the ID3v2 tag, it is TSOC (Picard>=1.3) and TXXX:COMPOSERSORT (Picard<=1.2). Since the official picard version is now 1.4.2, I used only the second, but that might cause problems. Concerned are beets/mediafile.py

Freso

You have a number of code style error, but AFAICT it's all whitespace, so fairly harmless. Still, better to not have all that stray whitespace. ;)

You add a couple of tests, but I'm not entirely sure what the tests are supposed to test, so also not sure whether they're needed at all. I will probably need @sampsyo to chime in that.

Also, for good measure, consider making a commit updating the changelog too. :)

(PS. You may also want to read up on writing good commit messages and making atomic commits.)

Thanks for your PR!

Freso · 2017-04-28T11:14:19Z

beets/autotag/__init__.py

    if track_info.arranger is not None:
        item.arranger = track_info.arranger
+
+


Just remove these blank lines.

Freso · 2017-04-28T11:14:55Z

beets/autotag/hooks.py

@@ -155,8 +156,8 @@ def __init__(self, title, track_id, artist=None, artist_id=None,
                 length=None, index=None, medium=None, medium_index=None,
                 medium_total=None, artist_sort=None, disctitle=None,
                 artist_credit=None, data_source=None, data_url=None,
-                 media=None, lyricist=None, composer=None, arranger=None,
-                 track_alt=None):
+                 media=None, lyricist=None, composer=None, composer_sort=None, 


There's a trailing space here.

Freso · 2017-04-28T11:16:01Z

beets/mediafile.py

@@ -1632,12 +1632,21 @@ def update(self, dict):
        StorageStyle('LYRICIST'),
        ASFStorageStyle('WM/Writer'),
    )
+


Follow the style of the rest of the code and don't add a blank line here.

Freso · 2017-04-28T11:16:08Z

beets/mediafile.py

    composer = MediaField(
        MP3StorageStyle('TCOM'),
        MP4StorageStyle('\xa9wrt'),
        StorageStyle('COMPOSER'),
        ASFStorageStyle('WM/Composer'),
    )
+


Freso · 2017-04-28T11:16:15Z

beets/mediafile.py

+        StorageStyle('COMPOSERSORT'),
+        ASFStorageStyle('WM/Composersortorder'),
+    )
+


Freso · 2017-04-28T11:27:33Z

test/test_mediafile.py

@@ -150,6 +156,12 @@ def test_add_image_structure(self):
        self.assertExtendedImageAttributes(
            image, desc=u'the composer', type=ImageType.composer
        )
+


Don't include whitespace on blank lines.

Freso · 2017-04-28T11:27:55Z

test/test_mediafile.py

+        image = next(images, None)
+        self.assertExtendedImageAttributes(
+            image, desc=u'the sortname of the composer', type=ImageType.composer_sort
+        )


Again, not sure this test is relevant here.

Freso · 2017-04-28T11:28:11Z

test/test_mediafile.py

@@ -193,6 +205,11 @@ def test_add_tiff_image(self):
                      type=ImageType.composer)
        mediafile.images += [image]
        mediafile.save()
+


Whitespace, blank line…

Freso · 2017-04-28T11:28:18Z

test/test_mediafile.py

@@ -202,6 +219,11 @@ def test_add_tiff_image(self):
                     mediafile.images))[0]
        self.assertExtendedImageAttributes(
            image, desc=u'the composer', type=ImageType.composer)
+


Whitespace, blank line…

Freso · 2017-04-28T11:29:28Z

test/test_mediafile.py

+        'album':         u'the album',
+        'genre':         u'the genre',
+        'composer':      u'the composer',
+        'composer_sort': u'the sortname of the composer',


I don't see artist_sort here, so maybe no need for composer_sort either?

true, but I wonder why there isn't artist_sort . Imo where there is a name there also should be the sort name.

anyway, removed it.

dosoe · 2017-04-28T11:38:43Z

I have nothing to do with the tests, they just popped randomly.

…r composer_sort; cleaning up whitespaces.

dosoe · 2017-04-28T11:46:54Z

'Also, for good measure, consider making a commit updating the changelog too. :)' What does that mean?

dosoe · 2017-04-28T11:47:19Z

and thanks for the links.

dosoe · 2017-04-28T12:14:17Z

by the way, as a familiar of musicbrainz, you might know how to add a "work" tag properly.

Freso · 2017-04-28T20:38:14Z

test/test_mediafile.py

@@ -373,7 +350,6 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin,
        'genre',
        'lyricist',
        'composer',
-        'composer_sort',


This list does have an artist_sort, so the composer_sort is probably also applicable here.

Freso · 2017-04-28T20:42:09Z

I have nothing to do with the tests, they just popped randomly.

That is not how tests work, sorry. The tests break either because changes in this pull request broke something that make existing tests fail, or because new tests added in the pull request are failing. If you really think there's randomness involved, you "Restart build" in Travis (not sure if you can manually trigger a rebuild in AppVeyor). The "Details" links under the "All checks have failed" should have more details you can use to try and figure out what code is failing once you've been convinced it isn't a fluke.

Code looks decent at a glance, but tests are failing and maybe some code needs to be readded based on feedback from @sampsyu.

dosoe · 2017-04-28T20:48:46Z

Ok, sorry, I should have been more clear. I know that these tests are not strictly speaking random, I just wanted to say that I have nothing to do with them (and btw I didn't add any test in this request).

jackwilsdon · 2017-05-12T00:05:45Z

beets/autotag/mb.py

@@ -218,10 +219,12 @@ def track_info(recording, index=None, medium=None, medium_index=None,
                    lyricist.append(artist_relation['artist']['name'])
                elif type == 'composer':
                    composer.append(artist_relation['artist']['name'])
+                    composer_sort.append(artist_relation['artist']['sort-name'])


This line is now too long, which is why our linting test is failing. You'll need to wrap it to be under 80 characters, the easiest way being this:

composer_sort.append( artist_relation['artist']['sort-name'])

sampsyo · 2017-05-12T16:42:34Z

Thank you! ✨ All merged up.

Add composer_sort as a tag

dosoe · 2017-05-12T16:47:09Z

My first merge!

dosoe added 3 commits April 28, 2017 12:10

add composer_sort tag#

c51ecd4

little indentation stuff

e3c3798

deleted one duplicate block

075e243

dosoe added 2 commits April 28, 2017 12:29

ASFStorageStyle corrected

2a418a6

adding image stuff for composer_sort

d4ff82e

Freso previously requested changes Apr 28, 2017

View reviewed changes

requested changes: where there is no artist_sort, there is no need fo…

4a17da8

…r composer_sort; cleaning up whitespaces.

if there is no artist_sort, there should not be a composer_sort.

23f172d

Freso reviewed Apr 28, 2017

View reviewed changes

added composer_sort on test_mediafile.py since there is artist_sort

813b078

dosoe mentioned this pull request May 11, 2017

add composer_sort and arranger_sort fields #2519

Closed

jackwilsdon requested changes May 12, 2017

View reviewed changes

Corrected line length

e444143

dosoe force-pushed the master branch from 0d300d0 to e444143 Compare May 12, 2017 09:38

jackwilsdon approved these changes May 12, 2017

View reviewed changes

sampsyo merged commit e444143 into beetbox:master May 12, 2017

sampsyo added a commit that referenced this pull request May 12, 2017

Merge pull request #2529 from dosoe/master

0cb643f

Add composer_sort as a tag

sampsyo added a commit that referenced this pull request May 12, 2017

Changelog for #2529, which fixes #2519

5eca518

dosoe mentioned this pull request May 16, 2017

Create arranger_sort, lyricist_sort, performer and performer_sort tags #2563

Closed

dosoe mentioned this pull request Jul 30, 2017

Add parentwork plugin #2580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add composer_sort as a tag #2529

Add composer_sort as a tag #2529

dosoe commented Apr 28, 2017

mention-bot commented Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

Freso left a comment

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

Freso Apr 28, 2017

dosoe Apr 28, 2017

dosoe Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

Freso Apr 28, 2017

Freso commented Apr 28, 2017

dosoe commented Apr 28, 2017

jackwilsdon May 12, 2017

sampsyo commented May 12, 2017

dosoe commented May 12, 2017

		if track_info.arranger is not None:
		item.arranger = track_info.arranger

Add composer_sort as a tag #2529

Add composer_sort as a tag #2529

Conversation

dosoe commented Apr 28, 2017

mention-bot commented Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

Freso 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

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

dosoe commented Apr 28, 2017

Choose a reason for hiding this comment

Freso commented Apr 28, 2017

dosoe commented Apr 28, 2017

Choose a reason for hiding this comment

sampsyo commented May 12, 2017

dosoe commented May 12, 2017