Skip to content
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

Make Bandcamp search result matching more robust #1553

Merged
merged 3 commits into from
Feb 24, 2024

Conversation

sferra
Copy link
Contributor

@sferra sferra commented Feb 16, 2024

Some metadata providers like Discogs provide track names which include trailing spaces which caused the search to fail. Removing leading and trailing spaces from track and artist names allows us to find any previously missed matches.

To reproduce the issue:

  • setup:
    • metadata provider: Discogs
    • stream provider: Bandcamp
  • search for "somali sun", select "The Sun" by "Somali Yacht Club"
  • no streams on Bandcamp match because all tracks contain trailing spaces and the matching is too strict

Some metadata providers like Discogs provide track names which include
traling spaces which caused the search to fail. Removing leading and
trailing spaces from track and artist names allows us to find any
previously missed matches.
@nuki-chan
Copy link

nuki-chan bot commented Feb 16, 2024

Lines 88-88

       tags: []
     };
     const streamQuery = {
-      artist: 'Artist Name',
-      track: 'Track Name',
+      artist: '  Artist Name',
+      track: 'Track Name  ',
       ...irrelevantSearchResultAttributes
     };

Ah, the classic case of "Let's sprinkle some extra whitespace and see who notices"! 😜 Dear developer-san, should we also assume that ' Mozart ' and 'Mozart' are from parallel universes? A trim() function in createTrackMatcher already exists to deal with such dastardly deeds. While Nuki appreciates white space in manga for dramatic effect, in code, not so much. The reference test case ought to stay trim and proper without leading or trailing spaces to reflect real-world usage. ✨

  • Update your streamQuery to remove the leading and trailing spaces:
     const streamQuery = {
-      artist: '  Artist Name',
-      track: 'Track Name  ',
+      artist: 'Artist Name',
+      track: 'Track Name',
       ...irrelevantSearchResultAttributes
     };

Lines 130-137

+
+  test('normalizes value for matching search results', () => {
+    // mixed case search term padded with spaces
+    const termToNormalize = '   Search Term   ';
+    const normalizedTerm = plugin.normalizeForMatching(termToNormalize);
+    expect(normalizedTerm).toEqual('search term');
+  });

Nuki sees what you did there! Slipping in a unit test for normalizeForMatching, smart cookie! 🍪👀 But darling, consider naming your test in a way that reflects the immense importance of what you're doing.

Why not something like:

test('normalizeForMatching trims and converts term to lowercase', () => {

It’s like giving a proper title to a light novel series, it has to be descriptive to pull the audience in, but like, don't make it a whole paragraph. 😽 Good job on testing the function independently, though. It shows you're taking testing seriously. Nuki is proud!


Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.61%. Comparing base (d01e199) to head (84fa709).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1553   +/-   ##
=======================================
  Coverage   71.60%   71.61%           
=======================================
  Files         364      364           
  Lines        6713     6714    +1     
  Branches      479      479           
=======================================
+ Hits         4807     4808    +1     
  Misses       1512     1512           
  Partials      394      394           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nukeop nukeop added the under review This pull request is being reviewed by maintainers. label Feb 16, 2024
@nukeop nukeop merged commit 1ec000a into nukeop:master Feb 24, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
under review This pull request is being reviewed by maintainers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants