-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #4080 #4085
Fix #4080 #4085
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this 🚀 I have a few concerns regarding the regex, see the inline comment.
I think I have the match.group correct I'll also try and look at tests to make sure we can check
I have added the following to the test_discogs.py file, but not sure how to actuqally call the code in discogs.py def test_album_for_id(self):
"""Test parsing for a valid Discogs release_id"""
test_patterns = [('http://www.discogs.com/G%C3%BCnther-Lause-Meru-Ep/release/4354798', 4354798),
('http://www.discogs.com/release/4354798-G%C3%BCnther-Lause-Meru-Ep', 4354798),
('http://www.discogs.com/G%C3%BCnther-4354798Lause-Meru-Ep/release/4354798', 4354798),
('http://www.discogs.com/release/4354798-G%C3%BCnther-4354798Lause-Meru-Ep/', 4354798),
('[r4354798]', 4354798),
('r4354798', 4354798),
('4354798', 4354798),
('yet-another-metadata-provider.org/foo/12345', ''),
('005b84a0-ecd6-39f1-b2f6-6eb48756b268', ''),
]
for test_pattern, expected in test_patterns:
for pattern in [
r'^\[?r?(?P<id>\d+)\]?$',
r'discogs\.com/release/(?P<id>\d+)-',
r'discogs\.com/[^/]+/release/(?P<id>\d+)',
]:
match = re.search(pattern, test_pattern)
if match:
match = int(match.group('id'))
break
if not match:
match = ''
self.assertEqual(match, expected) Do I need to redefine the album_for_id method in the test_discogs.py to contain the code required, otherwise I think we need to refactor album_for_id methods across plugins to pull the regex checks as a different method? |
I'd say moving the parsing to a separate method in the discogs plugin is easiest, i.e. move the new code to class DiscogsPlugin(BeetsPlugin):
# ...
@staticmethod
def extract_release_id(album_id):
# Here goes the new regex parsing, return either an integer id or None and call that from the new test (thanks by the way for adding one!) and from |
Have updated and added tests |
@wisp3rwind Thank you for your patience over the last few days. A learning experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is looking good! I have two more stylistic suggestions, the functionality looks fine to me now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay; this is looking really great. Thanks for working on this, everybody. (Moving the ID extraction to a separate, testable function was a particularly good idea.) 🎉
Description
Fixes #4080.
Tested with
https://www.discogs.com/SHOUSE-Love-Tonight-Robin-Schulz-Remix/release/20356324
https://www.discogs.com/release/20356324-SHOUSE-Love-Tonight-Robin-Schulz-Remix
Change REGEX
To Do
docs/
to describe it.)docs/changelog.rst
near the top of the document.)