Add parentwork plugin #2580

dosoe · 2017-05-31T15:59:38Z

Add a plugin that is able to fetch a performer, his sort name, the work, the work it is part of ( as explained in #2452 ) and the respective disambiguations, composer names and sort names.
This plugin doesn't work in its current form, I don't understand why (there is certainly a valuable reason and an easy fix, I'm just completely helpless when confronted with SQL).
I would be interested in additional featues or options for this plugin.

What it should do so far is:
fetch the performers of the recording
fetch the work title and disambiguation
look if the work is part of a bigger one, if yes take the work it is part of, repeat
fetch the parent work (which is part of no work) and disambiguation
fetch its composer (sort)name

Afaict the fetch part works fine, it's just the writing part that doesn't.
So far, I would only put the tags into the library, but if there is an actual tag out there where it can be written in the file itself I would be interested.

beetsplug/parentwork.py

beets/library.py

beetsplug/parentwork.py

sampsyo · 2017-06-01T02:31:07Z

beetsplug/parentwork.py

+            item['parent_composer']      = parent_composer
+            item['parent_composer_sort'] = parent_composer_sort
+
+            item.try_write()


If you're not planning on adding actual file tags for these fields, there's no need to call the write method.

Added flexible fields and corrected the typo of the tag: it was a list before for each tag, now it is a string. Also cleaned up a bit, but there is probably some whitespace to remove still or lines to break.

It seems that I should have lines shorter than 79 chars. However, I have so many nested loops or tests that this is hardly possible without removing the multiple tests for reaching the MB server. Normally, when beets tries to fetch something from MB using musicbrainzngs it has 8 tries and raises an error if it still doesn't work. However, for me, 8 tries are usually not enough (it doesn't work about half the times ) so I added more tries. This will be discarded because it creates two supplementary indentation blocks.

Put the search of the parentwork in a separate function, to allow to make longer lines.

dosoe · 2019-05-12T19:55:35Z

@sampsyo it seems like the tests are finally running fine. However, each time I want to edit the changelog I get a merging conflict, do you know why, or is it just that I didn't touch it for so long that I don't know how to use git anymore? Once this is cleared, could this be the moment to merge it?

arcresu

Thanks for updating your changes!

Your PR is trying to add 191 commits to beets, which seems like rather a lot. Could you look into rebasing to squash some of your intermediate commits? Perhaps hidden by this is that you've changed the file mode on some unrelated scripts like extra/release.py, which doesn't seem intended.

We reorganised the changelog file recently and something related to that is resulting in a confusing diff where it looks like your PR is re-applying those changes rather than just adding a new line. Something doesn't look right with the changelog.

EDIT: I realise you and Adrian discussed the commit count earlier, but I think the current situation is not ideal at least for commits that touch core files in beets. Sometimes we need to dig through the history of a file to debug or port changes, and it's made more difficult when the commit history contains many intermediate commits.

arcresu · 2019-05-12T23:36:51Z

beets/autotag/__init__.py

+        item.track_alt = track_info.track_alt
+
+        # Miscellaneous/nullable metadata.
+        misc_fields = {


Minor: this seems like it should be 2 variables misc_album_fields and misc_item_fields rather than a dict.

arcresu · 2019-05-12T23:39:08Z

beets/mediafile.py

+        MP4StorageStyle('----:com.apple.iTunes:Work'),
+        StorageStyle('WORK'),
+        ASFStorageStyle('beets/Work'),
+    )


We're planning to split MediaFile out into a standalone library so this addition will also need to be ported there. I can help with that if you like.

arcresu · 2019-05-12T23:39:33Z

beets/mediafile.py

+        MP3DescStorageStyle(u'Work'),
+        MP4StorageStyle('----:com.apple.iTunes:Work'),
+        StorageStyle('WORK'),
+        ASFStorageStyle('beets/Work'),


We probably don't want anything beets-specific in the storage style.

arcresu · 2019-05-12T23:41:07Z

beetsplug/parentwork.py

+
+def find_parentwork(work_id):
+    """This function gives the work relationships (dict) of a parent_work
+    given the id of the work"""


The docstring style used by beets needs the final """ to be on its own line.

arcresu · 2019-05-12T23:55:49Z

docs/plugins/parentwork.rst

+classical music, just fetching the work title as in MusicBrainz is not 
+satisfying, because MusicBrainz has separate works for, for example, all the 
+movements of a symphony. This plugin aims to solve this problem by not only 
+fetching the work as in MusicBrainz but also his parentwork which would be, 


Suggested change

fetching the work as in MusicBrainz but also his parentwork which would be,

fetching the work itself from MusicBrainz but also its parentwork which would be,

arcresu · 2019-05-12T23:57:00Z

docs/plugins/parentwork.rst

+Parentwork Plugin
+=================
+
+The `parentwork` plugin fetches the work title, parentwork title and 


Suggested change

The `parentwork` plugin fetches the work title, parentwork title and

The ``parentwork`` plugin fetches the work title, parentwork title and

arcresu · 2019-05-12T23:58:13Z

docs/plugins/parentwork.rst

+If there are several works linked to a recording, they all get a 
+disambiguation (empty as default) and if all disambiguations are empty, the 
+disambiguation field is left empty, else the disambiguation field can look 
+like `,disambig,,` (if there are four works and only the second has a 


Suggested change

like `,disambig,,` (if there are four works and only the second has a

like ``,disambig,,`` (if there are four works and only the second has a

arcresu · 2019-05-13T00:01:34Z

beetsplug/parentwork.py

+                            item.artist + ' - ' + item.title)
+            self._log.debug("Work fetched: " + u', '.join(parent_work) +
+                            ' - ' + u', '.join(parent_composer))
+            item['parent_work'] = u''


Lines like there aren't necessary since u', '.join([]) == u''

arcresu · 2019-05-13T00:06:45Z

beetsplug/parentwork.py

+        try:
+            item.parent_work
+            hasparent = True
+        except AttributeError:


Is an AttributeError really raised when accessing a flexible field?

sampsyo

Thanks for continuing to make progress on this effort!

At a very high level, what do you think about splitting up PRs for the new core functionality and for the plugin? I realize that one depends on the other, but this would make for somewhat easier reviewing and could be part of the flattening that @arcresu brought up (and which I agree might be a good idea at this point).

sampsyo · 2019-05-13T01:48:01Z

beetsplug/parentwork.py

+
+
+def work_father(work_id, work_date=None):
+    """ This function finds the id of the father work given its id"""


It seems like this docstring should fully describe the return type: namely, it's a pair of an ID and a date. It's also probably worth mentioning that this communicates with MusicBrainz.

sampsyo · 2019-05-13T01:49:26Z

beetsplug/parentwork.py

+    work_info = musicbrainzngs.get_work_by_id(work_id,
+                                              includes=["work-rels",
+                                                        "artist-rels"])
+    if 'artist-relation-list' in work_info['work'] and work_date is None:


A comment here could be useful to point out that what we're doing is filling in work_date if it's unspecified (how?).

sampsyo · 2019-05-13T01:49:51Z

beetsplug/parentwork.py

+    """This function finds the parentwork id of a work given its id. """
+    work_date = None
+    while True:
+        (new_work_id, work_date) = work_father(work_id, work_date)


No parentheses are necessary on the left-hand side.

sampsyo · 2019-05-13T01:50:01Z

beetsplug/parentwork.py

+        (new_work_id, work_date) = work_father(work_id, work_date)
+        if not new_work_id:
+            return work_id, work_date
+            break


This break will never be executed.

sampsyo · 2019-05-13T01:50:37Z

beetsplug/parentwork.py

+                    and work_father.get('direction') == 'backward':
+                father_id = work_father['work']['id']
+                return father_id, work_date
+    return None, work_date


What does it mean to return None as the first part of the tuple here?

sampsyo · 2019-05-13T01:52:21Z

beetsplug/parentwork.py

+        self._command.parser.add_option(
+            u'-f', u'--force', dest='force',
+            action='store_true', default=None,
+            help=u'Re-fetches all parent works')


sampsyo · 2019-05-13T01:53:16Z

beetsplug/parentwork.py

+                 composer_ids):
+        """Given the parentwork info dict, this function updates
+        parent_composer, parent_composer_sort, parent_work,
+        parent_work_disambig, work_ids and composer_ids"""


I'd prefer for this function to return information rather than modifying its arguments. The caller can then update all the arrays to collect the data across multiple works.

sampsyo · 2019-05-13T01:53:52Z

beetsplug/parentwork.py

+            self._log.info(item.artist + ' - ' + item.title)
+            self._log.info(
+                "no composer, add one at https://musicbrainz.org/work/" +
+                work_info['work']['id'])


I don't think we need this log message—if anything, this can go to the debug log.

sampsyo · 2019-05-13T01:56:31Z

beetsplug/parentwork.py

+            item['parent_composer'] = u', '.join(parent_composer)
+            item['parent_composer_sort'] = u''
+            item['parent_composer_sort'] = u', '.join(parent_composer_sort)
+            if not (work_date is None):


Suggested change

if not (work_date is None):

if work_date:

sampsyo · 2019-05-13T01:56:58Z

beetsplug/parentwork.py

+                            ' - ' + u', '.join(parent_composer))
+            item['parent_work'] = u''
+            item['parent_work'] = u', '.join(parent_work)
+            if len(parent_work_disambig) > 0:


Suggested change

if len(parent_work_disambig) > 0:

if parent_work_disambig:

dosoe · 2019-05-13T12:42:37Z

Hi! Thanks for the feedback! I'm not familiar with git or github, so I tried rebasing, but without success I think. Could an easy solution be for me to restart from scratch and decompose it accordingly into different PRs? Splitting between changes to the core and the plugin might be tricky considering that the plugin relies on the changes to the core.

sampsyo · 2019-05-13T12:47:57Z

New PRs would work just fine! You could even start with the core fields PR and base the plugin PR on that branch to encode the fact that the latter depends on the former.

dosoe added 2 commits May 31, 2017 17:43

add work-related tags to library

6e08fcf

the actual parentwork plugin

984b80f

This comment has been minimized.

Sign in to view

dosoe commented May 31, 2017

View reviewed changes

beetsplug/parentwork.py Outdated Show resolved Hide resolved

dosoe commented May 31, 2017

View reviewed changes

beetsplug/parentwork.py Outdated Show resolved Hide resolved

dosoe commented May 31, 2017

View reviewed changes

beetsplug/parentwork.py Show resolved Hide resolved

sampsyo reviewed Jun 1, 2017

View reviewed changes

dosoe added 3 commits June 1, 2017 14:25

No field list necessary when storing; no writing on files possible

075fa5b

removed work-related tags for flexible field typeS

f4cd19f

Added flexible fields and corrected types

afa670b

Added flexible fields and corrected the typo of the tag: it was a list before for each tag, now it is a string. Also cleaned up a bit, but there is probably some whitespace to remove still or lines to break.

This comment has been minimized.

Sign in to view

Get it work autonomously: typo

5b0b650

This comment has been minimized.

Sign in to view

dosoe added 8 commits June 1, 2017 17:05

style

d191316

style

49a126f

style

6a1ac92

style

d778fa7

style

8a03b42

style

074a0cd

corrected performer fetching

a6b98f7

no printing

6484e04

This comment has been minimized.

Sign in to view

dosoe added 3 commits June 3, 2017 13:11

put search of parentwork in separate function

a184aa3

Put the search of the parentwork in a separate function, to allow to make longer lines.

debug message

cd292fa

less printing, more precise debug messages

8282417

This comment has been minimized.

Sign in to view

dosoe added 2 commits August 27, 2018 10:43

adding next test

f0d05a6

adding next test

c20ed9b

This comment has been minimized.

Sign in to view

dosoe added 6 commits August 27, 2018 11:16

remove option tests

ce8c86b

remove option tests

54d91d4

just test for normal case

f9dd6e7

typo

bb89356

just test the normal case

de3382f

reinitialize fields if force

4d07b45

arcresu added the newplugin label Apr 7, 2019

arcresu mentioned this pull request Apr 27, 2019

mediafile: replace with a re-export of beetbox/mediafile #3237

Merged

7 tasks

dosoe added 11 commits May 12, 2019 20:12

Corrected conditions to write to database

915268e

not empty fields for disambiguations and work dates

ba580c0

Sync up with main fork

6987d54

synching up with main fork

c05ea53

check tests

8d6fc12

flake8

4d7489a

tests run fine, update changelog.

fbc81df

flake8

75de298

flake8

38c4368

merging conflicts

e86f510

flake8

8b005ac

arcresu reviewed May 13, 2019

View reviewed changes

sampsyo requested changes May 13, 2019

View reviewed changes

This was referenced May 25, 2019

add work, work-disambig and work_id tags #3272

Merged

add parentwork plugin #3279

Merged

sampsyo closed this in #3279 Jun 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parentwork plugin #2580

Add parentwork plugin #2580

dosoe commented May 31, 2017

This comment has been minimized.

This comment has been minimized.

sampsyo Jun 1, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dosoe commented May 12, 2019

arcresu left a comment •

edited

Loading

arcresu May 12, 2019

arcresu May 12, 2019

arcresu May 12, 2019

arcresu May 12, 2019

arcresu May 12, 2019

arcresu May 12, 2019

arcresu May 12, 2019

arcresu May 13, 2019

arcresu May 13, 2019

sampsyo left a comment

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

sampsyo May 13, 2019

dosoe commented May 13, 2019

sampsyo commented May 13, 2019

	fetching the work as in MusicBrainz but also his parentwork which would be,
	fetching the work itself from MusicBrainz but also its parentwork which would be,

	The `parentwork` plugin fetches the work title, parentwork title and
	The ``parentwork`` plugin fetches the work title, parentwork title and

	like `,disambig,,` (if there are four works and only the second has a
	like ``,disambig,,`` (if there are four works and only the second has a



		def work_father(work_id, work_date=None):
		""" This function finds the id of the father work given its id"""

Add parentwork plugin #2580

Add parentwork plugin #2580

Conversation

dosoe commented May 31, 2017

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dosoe commented May 12, 2019

arcresu left a comment • edited Loading

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

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

dosoe commented May 13, 2019

sampsyo commented May 13, 2019

arcresu left a comment •

edited

Loading