From b914b311196d02e683ca98dbea984d4dd59d2895 Mon Sep 17 00:00:00 2001 From: ABCbum Date: Wed, 18 Dec 2019 13:25:00 +0700 Subject: [PATCH 1/3] Enable mblookup to take release id as argument To make mblookup able to look up data based on release id, RegExp is used to detect whether the input is release id or disc id and behaves differently according to that. The input is now also tripped before being passed down. Signed-off-by: ABCbum --- whipper/command/mblookup.py | 71 +++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/whipper/command/mblookup.py b/whipper/command/mblookup.py index 9ecbfba..044fab7 100644 --- a/whipper/command/mblookup.py +++ b/whipper/command/mblookup.py @@ -1,5 +1,7 @@ from whipper.command.basecommand import BaseCommand -from whipper.common.mbngs import musicbrainz +from whipper.common.mbngs import musicbrainz, getReleaseMetadata + +import re class MBLookup(BaseCommand): @@ -12,35 +14,58 @@ Example disc id: KnpGsLhvH.lPrNc1PBL21lb9Bg4-""" def add_arguments(self): self.parser.add_argument( - 'mbdiscid', action='store', help="MB disc id to look up" + 'mbid', action='store', help="MB disc id or release id to look up" ) + def _printMetadata(self, md): + """ + Print out metadata received in a sensible way. + + :param md: MusicBrainz's metadata about the disc + :type md: `DiscMetadata` + """ + print(' Artist: %s' % md.artist.encode('utf-8')) + print(' Title: %s' % md.title.encode('utf-8')) + print(' Type: %s' % str(md.releaseType).encode('utf-8')) + print(' URL: %s' % md.url) + print(' Tracks: %d' % len(md.tracks)) + if md.catalogNumber: + print(' Cat no: %s' % md.catalogNumber) + if md.barcode: + print(' Barcode: %s' % md.barcode) + + for j, track in enumerate(md.tracks): + print(' Track %2d: %s - %s' % ( + j + 1, track.artist.encode('utf-8'), + track.title.encode('utf-8') + )) + def do(self): try: - discId = str(self.options.mbdiscid) + mbid = str(self.options.mbid.strip()) except IndexError: - print('Please specify a MusicBrainz disc id.') + print('Please specify a MusicBrainz disc id or release id.') return 3 - metadatas = musicbrainz(discId) + releaseIdMatch = re.match( + r'^[\dA-Fa-f]{8}-(?:[\dA-Fa-f]{4}-){3}[\dA-Fa-f]{12}$', + mbid + ) + discIdMatch = re.match( + r'^[\dA-Za-z._]{27}-$', + mbid + ) - print('%d releases' % len(metadatas)) - for i, md in enumerate(metadatas): - print('- Release %d:' % (i + 1, )) - print(' Artist: %s' % md.artist.encode('utf-8')) - print(' Title: %s' % md.title.encode('utf-8')) - print(' Type: %s' % str(md.releaseType).encode('utf-8')) # noqa: E501 - print(' URL: %s' % md.url) - print(' Tracks: %d' % len(md.tracks)) - if md.catalogNumber: - print(' Cat no: %s' % md.catalogNumber) - if md.barcode: - print(' Barcode: %s' % md.barcode) - - for j, track in enumerate(md.tracks): - print(' Track %2d: %s - %s' % ( - j + 1, track.artist.encode('utf-8'), - track.title.encode('utf-8') - )) + # see https://musicbrainz.org/doc/MusicBrainz_Identifier + if releaseIdMatch: + md = getReleaseMetadata(releaseIdMatch.group(0)) + if md: + self._printMetadata(md) + elif discIdMatch: + metadatas = musicbrainz(discIdMatch.group(0)) + print('%d releases' % len(metadatas)) + for i, md in enumerate(metadatas): + print('- Release %d:' % (i + 1, )) + self._printMetadata(md) return None From 78c91fd1c7a5e0620f1da44ff202e2038dcdbc31 Mon Sep 17 00:00:00 2001 From: ABCbum Date: Wed, 18 Dec 2019 13:30:08 +0700 Subject: [PATCH 2/3] Add new functionality and refactor code In order to make mblookup command able to lookup data based on release id, new function getReleaseMetadata is created. To remove duplicated code, importing and set_useragent is moved to the top of the file. Now _getMetadata behaves differently when the discid is not specified. Signed-off-by: ABCbum --- whipper/common/mbngs.py | 61 +++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 20 deletions(-) diff --git a/whipper/common/mbngs.py b/whipper/common/mbngs.py index 2029c61..95967bb 100644 --- a/whipper/common/mbngs.py +++ b/whipper/common/mbngs.py @@ -24,9 +24,13 @@ Handles communication with the MusicBrainz server using NGS. from urllib.error import HTTPError import whipper +import json +import musicbrainzngs import logging logger = logging.getLogger(__name__) +musicbrainzngs.set_useragent("whipper", whipper.__version__, + "https://github.com/whipper-team/whipper") VA_ID = "89ad4ac3-39f7-470e-963a-56509c546377" # Various Artists @@ -161,7 +165,7 @@ def _getWorks(recording): return works -def _getMetadata(release, discid, country=None): +def _getMetadata(release, discid=None, country=None): """ :type release: dict :param release: a release dict as returned in the value for key release @@ -220,7 +224,7 @@ def _getMetadata(release, discid, country=None): # only show discs from medium-list->disc-list with matching discid for medium in release['medium-list']: for disc in medium['disc-list']: - if disc['id'] == discid: + if discid is None or disc['id'] == discid: title = release['title'] discMD.releaseTitle = title if 'disambiguation' in release: @@ -271,6 +275,40 @@ def _getMetadata(release, discid, country=None): return discMD +def getReleaseMetadata(release_id, discid=None, country=None, record=False): + """ + Return a DiscMetadata object based on MusicBrainz Release ID and Disc ID. + + If the disc id is not specified, it will match with any disc that is on + the release disc-list. Otherwise only returns metadata of one disc in + release disc-list. + + :param release_id: MusicBrainz Release ID + :type release_id: str + :param discid: MusicBrainz Disc ID + :type discid: str or None + :param country: the country the release was issued in + :type country: str or None + :param record: whether to record to disc as a JSON serialization + :type record: bool + :returns: a DiscMetadata object based on MusicBrainz Release ID & Disc ID + :rtype: `DiscMetadata` + """ + # to get titles of recordings, we need to query the release with + # artist-credits + + res = musicbrainzngs.get_release_by_id( + release_id, includes=["artists", "artist-credits", + "recordings", "discids", + "labels", "recording-level-rels", + "work-rels", "release-groups"]) + _record(record, 'release', release_id, res) + releaseDetail = res['release'] + formatted = json.dumps(releaseDetail, sort_keys=False, indent=4) + logger.debug('release %s', formatted) + return _getMetadata(releaseDetail, discid, country) + + # see http://bugs.musicbrainz.org/browser/python-musicbrainz2/trunk/examples/ # ripper.py @@ -287,11 +325,8 @@ def musicbrainz(discid, country=None, record=False): :rtype: list of :any:`DiscMetadata` """ logger.debug('looking up results for discid %r', discid) - import musicbrainzngs logging.getLogger("musicbrainzngs").setLevel(logging.WARNING) - musicbrainzngs.set_useragent("whipper", whipper.__version__, - "https://github.com/whipper-team/whipper") ret = [] try: @@ -314,26 +349,12 @@ def musicbrainz(discid, country=None, record=False): # Display the returned results to the user. - import json for release in result['disc']['release-list']: formatted = json.dumps(release, sort_keys=False, indent=4) logger.debug('result %s: artist %r, title %r', formatted, release['artist-credit-phrase'], release['title']) - # to get titles of recordings, we need to query the release with - # artist-credits - - res = musicbrainzngs.get_release_by_id( - release['id'], includes=["artists", "artist-credits", - "recordings", "discids", "labels", - "recording-level-rels", "work-rels", - "release-groups"]) - _record(record, 'release', release['id'], res) - releaseDetail = res['release'] - formatted = json.dumps(releaseDetail, sort_keys=False, indent=4) - logger.debug('release %s', formatted) - - md = _getMetadata(releaseDetail, discid, country) + md = getReleaseMetadata(release['id'], discid, country, record) if md: logger.debug('duration %r', md.duration) ret.append(md) From bb66a092cd586546ecf12159ba1375f6945ce3a6 Mon Sep 17 00:00:00 2001 From: ABCbum Date: Wed, 18 Dec 2019 13:34:38 +0700 Subject: [PATCH 3/3] Add test case to new mblookup functionality A new test case to check for mblookup's ability to search for data based on release id is created along with a function mocks getReleaseMetadata using an existing JSON file. Signed-off-by: ABCbum --- whipper/test/test_command_mblookup.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/whipper/test/test_command_mblookup.py b/whipper/test/test_command_mblookup.py index c13cb93..39d058a 100644 --- a/whipper/test/test_command_mblookup.py +++ b/whipper/test/test_command_mblookup.py @@ -4,8 +4,10 @@ import os import pickle import unittest +import json from whipper.command import mblookup +from whipper.common.mbngs import _getMetadata class MBLookupTestCase(unittest.TestCase): @@ -19,6 +21,22 @@ class MBLookupTestCase(unittest.TestCase): with open(path, "rb") as p: return pickle.load(p) + @staticmethod + def _mock_getReleaseMetadata(release_id): + """ + Mock function for whipper.common.mbngs.getReleaseMetadata. + + :param release_id: MusicBrainz Release ID + :type release_id: str + :returns: a DiscMetadata object based on the given release_id + :rtype: `DiscMetadata` + """ + filename = 'whipper.release.{}.json'.format(release_id) + path = os.path.join(os.path.dirname(__file__), filename) + with open(path, "rb") as handle: + response = json.loads(handle.read().decode('utf-8')) + return _getMetadata(response['release']) + def testMissingReleaseType(self): """Test that lookup for release without a type set doesn't fail.""" # Using: Gustafsson, Österberg & Cowle - What's Up? 8 (disc 4) @@ -28,3 +46,12 @@ class MBLookupTestCase(unittest.TestCase): # https://musicbrainz.org/cdtoc/xu338_M8WukSRi0J.KTlDoflB8Y- lookup = mblookup.MBLookup([discid], 'whipper mblookup', None) lookup.do() + + def testGetDataFromReleaseId(self): + """Test that lookup for a release with a specified id.""" + # Using: The KLF - Space & Chill Out + # https://musicbrainz.org/release/c56ff16e-1d81-47de-926f-ba22891bd2bd + mblookup.getReleaseMetadata = self._mock_getReleaseMetadata + releaseid = 'c56ff16e-1d81-47de-926f-ba22891bd2bd' + lookup = mblookup.MBLookup([releaseid], 'whipper mblookup', None) + lookup.do()