From 846ee04a5f9627672a85fc59a4168a4b5bb450bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 13:02:18 +0200 Subject: [PATCH 01/11] Add test case illustrating https://github.com/JoeLametta/whipper/issues/127 --- whipper/test/test_common_program.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/whipper/test/test_common_program.py b/whipper/test/test_common_program.py index 170bbcc..b67e13e 100644 --- a/whipper/test/test_common_program.py +++ b/whipper/test/test_common_program.py @@ -119,3 +119,21 @@ class PathTestCase(unittest.TestCase): path = prog.getPath(u'/tmp', u'%A/%d', 'mbdiscid', 0) self.assertEquals(path, u'/tmp/Jeff Buckley/Grace') + + def testDisambiguateOnRelease(self): + """Test that disambiguation gets placed in the same part of the path as the release name. + + See https://github.com/JoeLametta/whipper/issues/127""" + prog = program.Program(config.Config()) + md = mbngs.DiscMetadata() + md.artist = 'Guy Davis' + md.sortName = 'Davis, Guy' + md.title = 'Call Down the Thunder' + md.release = '1996' + md.catalogNumber = 'RHR CD 89' + template = u'%A/%d - %y' + prog.metadata = md + + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + self.assertEquals(path, + u'/tmp/Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)') From 3f30f1d46f407d145d0c51a0d435c54172e9a6ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 13:54:43 +0200 Subject: [PATCH 02/11] Always disambiguate in the release title part of the template Fixes https://github.com/JoeLametta/whipper/issues/127 --- whipper/common/program.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/whipper/common/program.py b/whipper/common/program.py index 5f084b8..62e9ec8 100644 --- a/whipper/common/program.py +++ b/whipper/common/program.py @@ -243,10 +243,13 @@ class Program: # when disambiguating, use catalogNumber then barcode if disambiguate: templateParts = list(os.path.split(template)) - if self.metadata.catalogNumber: - templateParts[-2] += ' (%s)' % self.metadata.catalogNumber - elif self.metadata.barcode: - templateParts[-2] += ' (%s)' % self.metadata.barcode + # Find the section of the template with the release name + for i, part in enumerate(templateParts): + if "%d" in part: + if self.metadata.catalogNumber: + templateParts[i] += ' (%s)' % self.metadata.catalogNumber + elif self.metadata.barcode: + templateParts[i] += ' (%s)' % self.metadata.barcode template = os.path.join(*templateParts) logger.debug('Disambiguated template to %r' % template) From 12e52da55d9109c0eec25c26eefab651b626667e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 14:18:21 +0200 Subject: [PATCH 03/11] Add more template test cases to testDisambiguateOnRelease Some of these might not occur in the wild, but we theoretically support them at least, and making sure they work might catch other issues we didn't think of yet. --- whipper/test/test_common_program.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/whipper/test/test_common_program.py b/whipper/test/test_common_program.py index b67e13e..62cef64 100644 --- a/whipper/test/test_common_program.py +++ b/whipper/test/test_common_program.py @@ -131,9 +131,15 @@ class PathTestCase(unittest.TestCase): md.title = 'Call Down the Thunder' md.release = '1996' md.catalogNumber = 'RHR CD 89' - template = u'%A/%d - %y' prog.metadata = md + templates = { + u'%A/%d - %y': u'Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)', + u'%A - %d - %y': u'Guy Davis - Call Down the Thunder - 1996 (RHR CD 89)', + u'%A/%y/%d': u'Guy Davis/1996/Call Down the Thunder (RHR CD 89)', + u'%y/%d/%A': u'1996/Call Down the Thunder (RHR CD 89)/Guy Davis', + u'%d/%A/%y': u'Call Down the Thunder (RHR CD 89)/Guy Davis/1996', + } - path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) - self.assertEquals(path, - u'/tmp/Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)') + for template, expected_path in templates.iteritems(): + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + self.assertEquals(path, u'/tmp/' + expected_path) From a14989583ef2e1726f0005d9e75c960475f4a72d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 14:25:17 +0200 Subject: [PATCH 04/11] Use regular str.split() to split template parts in disambiguation os.path.split() only even splits into two components, which means that path templates that have more than two parts (e.g., `%A/%d - %y/%X`) will not get split properly for the purpose of added disambiguation parts to them. os.path.join() will still work fine to splice the split template back together as it takes an arbitrary number of arguments. --- whipper/common/program.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/whipper/common/program.py b/whipper/common/program.py index 62e9ec8..8d6bebf 100644 --- a/whipper/common/program.py +++ b/whipper/common/program.py @@ -242,7 +242,7 @@ class Program: # when disambiguating, use catalogNumber then barcode if disambiguate: - templateParts = list(os.path.split(template)) + templateParts = template.split(os.sep) # Find the section of the template with the release name for i, part in enumerate(templateParts): if "%d" in part: From f3e3748d7570a8a514650dfbc844570e7e678d08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 14:29:53 +0200 Subject: [PATCH 05/11] Test that disambiguation is only added once --- whipper/test/test_common_program.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/whipper/test/test_common_program.py b/whipper/test/test_common_program.py index 62cef64..41b87cd 100644 --- a/whipper/test/test_common_program.py +++ b/whipper/test/test_common_program.py @@ -143,3 +143,19 @@ class PathTestCase(unittest.TestCase): for template, expected_path in templates.iteritems(): path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) self.assertEquals(path, u'/tmp/' + expected_path) + + def testDisambiguateOnReleaseOnlyOnce(self): + """Test that disambiguation gets added only once.""" + prog = program.Program(config.Config()) + md = mbngs.DiscMetadata() + md.artist = 'Guy Davis' + md.sortName = 'Davis, Guy' + md.title = 'Call Down the Thunder' + md.release = '1996' + md.catalogNumber = 'RHR CD 89' + prog.metadata = md + template = u'%A/%d - %y/%d/%d' + + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + self.assertEquals(path, + u'/tmp/Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)/Call Down the Thunder/Call Down the Thunder') From e5f2afe0dbf2a39d6b697ab9b418507d4320951d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 14:34:07 +0200 Subject: [PATCH 06/11] Break out of disambiguation loop once disambig has been added Instead of iterating over all the path parts and added disambiguation to each, it now only adds the disambiguation to the first match. --- whipper/common/program.py | 1 + 1 file changed, 1 insertion(+) diff --git a/whipper/common/program.py b/whipper/common/program.py index 8d6bebf..9791ae8 100644 --- a/whipper/common/program.py +++ b/whipper/common/program.py @@ -250,6 +250,7 @@ class Program: templateParts[i] += ' (%s)' % self.metadata.catalogNumber elif self.metadata.barcode: templateParts[i] += ' (%s)' % self.metadata.barcode + break template = os.path.join(*templateParts) logger.debug('Disambiguated template to %r' % template) From 4512cd39197c3165205a1539fd3605bf82f90dcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 14:44:19 +0200 Subject: [PATCH 07/11] Test that disambigation is added to templates without "%d" --- whipper/test/test_common_program.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/whipper/test/test_common_program.py b/whipper/test/test_common_program.py index 41b87cd..d31bd67 100644 --- a/whipper/test/test_common_program.py +++ b/whipper/test/test_common_program.py @@ -159,3 +159,23 @@ class PathTestCase(unittest.TestCase): path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) self.assertEquals(path, u'/tmp/Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)/Call Down the Thunder/Call Down the Thunder') + + def testDisambiguateOnNoReleaseTitle(self): + """Test that disambiguation gets added even if there's no release title in the template.""" + prog = program.Program(config.Config()) + md = mbngs.DiscMetadata() + md.artist = 'Guy Davis' + md.sortName = 'Davis, Guy' + md.title = 'Call Down the Thunder' + md.release = '1996' + md.catalogNumber = 'RHR CD 89' + prog.metadata = md + templates = { + u'%A/%y': u'Guy Davis/1996 (RHR CD 89)', + u'%A - %y': u'Guy Davis - 1996 (RHR CD 89)', + u'%y/%A': u'1996/Guy Davis (RHR CD 89)', + } + + for template, expected_path in templates.iteritems(): + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + self.assertEquals(path, u'/tmp/' + expected_path) From 2465f03337c5b71ed761c0d7cb73c9dbe4d9d7cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 14:45:51 +0200 Subject: [PATCH 08/11] Add disambiguation to the end of template strings w/o "%d" --- whipper/common/program.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/whipper/common/program.py b/whipper/common/program.py index 9791ae8..93118be 100644 --- a/whipper/common/program.py +++ b/whipper/common/program.py @@ -251,6 +251,12 @@ class Program: elif self.metadata.barcode: templateParts[i] += ' (%s)' % self.metadata.barcode break + else: + # No parts of the template contain the release + if self.metadata.catalogNumber: + templateParts[-1] += ' (%s)' % self.metadata.catalogNumber + elif self.metadata.barcode: + templateParts[-1] += ' (%s)' % self.metadata.barcode template = os.path.join(*templateParts) logger.debug('Disambiguated template to %r' % template) From b0d047ded1a9e80ca66a44c11b178cf20cd99974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 15:10:08 +0200 Subject: [PATCH 09/11] Add unittest for new Program.addDisambiguation() method --- whipper/test/test_common_program.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/whipper/test/test_common_program.py b/whipper/test/test_common_program.py index d31bd67..9c33c86 100644 --- a/whipper/test/test_common_program.py +++ b/whipper/test/test_common_program.py @@ -179,3 +179,25 @@ class PathTestCase(unittest.TestCase): for template, expected_path in templates.iteritems(): path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) self.assertEquals(path, u'/tmp/' + expected_path) + + def testAddDisambiguationUnitTest(self): + """Unit test for Program.addDisambiguation().""" + prog = program.Program(config.Config()) + md = mbngs.DiscMetadata() + + # No relevant disambiguation metadata + self.assertEquals( + prog.addDisambiguation(u'Test', md), + u'Test') + + # Only barcode available + md.barcode = '033651008927' + self.assertEquals( + prog.addDisambiguation(u'Test', md), + u'Test (033651008927)') + + # Both catalog number and barcode available + md.catalogNumber = 'RHR CD 89' + self.assertEquals( + prog.addDisambiguation(u'Test', md), + u'Test (RHR CD 89)') From 5ba3e924fbeeac11f605acb4b5f783857146d18f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Sat, 29 Apr 2017 15:45:14 +0200 Subject: [PATCH 10/11] Refactor Program.getPath disambiguation logic to .addDisambiguation --- whipper/common/program.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/whipper/common/program.py b/whipper/common/program.py index 93118be..1c186af 100644 --- a/whipper/common/program.py +++ b/whipper/common/program.py @@ -170,6 +170,14 @@ class Program: def saveRipResult(self): self._presult.persist() + def addDisambiguation(self, template_part, metadata): + "Add disambiguation to template path part string." + if metadata.catalogNumber: + template_part += ' (%s)' % metadata.catalogNumber + elif metadata.barcode: + template_part += ' (%s)' % metadata.barcode + return template_part + def getPath(self, outdir, template, mbdiscid, i, disambiguate=False): """ Based on the template, get a complete path for the given track, @@ -246,17 +254,11 @@ class Program: # Find the section of the template with the release name for i, part in enumerate(templateParts): if "%d" in part: - if self.metadata.catalogNumber: - templateParts[i] += ' (%s)' % self.metadata.catalogNumber - elif self.metadata.barcode: - templateParts[i] += ' (%s)' % self.metadata.barcode + templateParts[i] = self.addDisambiguation(part, self.metadata) break else: # No parts of the template contain the release - if self.metadata.catalogNumber: - templateParts[-1] += ' (%s)' % self.metadata.catalogNumber - elif self.metadata.barcode: - templateParts[-1] += ' (%s)' % self.metadata.barcode + templateParts[-1] = self.addDisambiguation(templateParts[-1], self.metadata) template = os.path.join(*templateParts) logger.debug('Disambiguated template to %r' % template) From a0c0ce7e479329b58f70d684d54ef590995f395a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20=E2=80=9CFreso=E2=80=9D=20S=2E=20Olesen?= Date: Thu, 1 Jun 2017 02:31:03 +0200 Subject: [PATCH 11/11] Fix(/ignore) PEP8/flake8 errors --- whipper/common/program.py | 4 ++-- whipper/test/test_common_program.py | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/whipper/common/program.py b/whipper/common/program.py index 1c186af..ea223de 100644 --- a/whipper/common/program.py +++ b/whipper/common/program.py @@ -254,11 +254,11 @@ class Program: # Find the section of the template with the release name for i, part in enumerate(templateParts): if "%d" in part: - templateParts[i] = self.addDisambiguation(part, self.metadata) + templateParts[i] = self.addDisambiguation(part, self.metadata) # noqa: E501 break else: # No parts of the template contain the release - templateParts[-1] = self.addDisambiguation(templateParts[-1], self.metadata) + templateParts[-1] = self.addDisambiguation(templateParts[-1], self.metadata) # noqa: E501 template = os.path.join(*templateParts) logger.debug('Disambiguated template to %r' % template) diff --git a/whipper/test/test_common_program.py b/whipper/test/test_common_program.py index 9c33c86..dfa1867 100644 --- a/whipper/test/test_common_program.py +++ b/whipper/test/test_common_program.py @@ -121,7 +121,8 @@ class PathTestCase(unittest.TestCase): u'/tmp/Jeff Buckley/Grace') def testDisambiguateOnRelease(self): - """Test that disambiguation gets placed in the same part of the path as the release name. + """Test that disambiguation gets placed in the same part of the path + as the release name. See https://github.com/JoeLametta/whipper/issues/127""" prog = program.Program(config.Config()) @@ -133,15 +134,15 @@ class PathTestCase(unittest.TestCase): md.catalogNumber = 'RHR CD 89' prog.metadata = md templates = { - u'%A/%d - %y': u'Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)', - u'%A - %d - %y': u'Guy Davis - Call Down the Thunder - 1996 (RHR CD 89)', + u'%A/%d - %y': u'Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)', # noqa: E501 + u'%A - %d - %y': u'Guy Davis - Call Down the Thunder - 1996 (RHR CD 89)', # noqa: E501 u'%A/%y/%d': u'Guy Davis/1996/Call Down the Thunder (RHR CD 89)', u'%y/%d/%A': u'1996/Call Down the Thunder (RHR CD 89)/Guy Davis', u'%d/%A/%y': u'Call Down the Thunder (RHR CD 89)/Guy Davis/1996', } for template, expected_path in templates.iteritems(): - path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) # noqa: E501 self.assertEquals(path, u'/tmp/' + expected_path) def testDisambiguateOnReleaseOnlyOnce(self): @@ -156,12 +157,13 @@ class PathTestCase(unittest.TestCase): prog.metadata = md template = u'%A/%d - %y/%d/%d' - path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) # noqa: E501 self.assertEquals(path, - u'/tmp/Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)/Call Down the Thunder/Call Down the Thunder') + u'/tmp/Guy Davis/Call Down the Thunder - 1996 (RHR CD 89)/Call Down the Thunder/Call Down the Thunder') # noqa: E501 def testDisambiguateOnNoReleaseTitle(self): - """Test that disambiguation gets added even if there's no release title in the template.""" + """Test that disambiguation gets added even if there's no release + title in the template.""" prog = program.Program(config.Config()) md = mbngs.DiscMetadata() md.artist = 'Guy Davis' @@ -177,7 +179,7 @@ class PathTestCase(unittest.TestCase): } for template, expected_path in templates.iteritems(): - path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) + path = prog.getPath(u'/tmp', template, 'mbdiscid', 0, disambiguate=True) # noqa: E501 self.assertEquals(path, u'/tmp/' + expected_path) def testAddDisambiguationUnitTest(self):