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

Fix imprecise stream search in the Bandcamp plugin #1549

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

sferra
Copy link
Contributor

@sferra sferra commented Feb 11, 2024

When the name of a track was very simple, the actual search match could be on a page higher than the configured maximum. This caused the search to fail and the track wouldn't be played. The amount of search results can be greatly reduced by simply adding the artist name to the query.

To reproduce the issue:

  • search for "somali yacht club sun"
  • select the album "The Sun" by "Somali Yacht Club"
  • attempt to play the first track: "Loom". Playing will fail, because when searching for "Loom" alone, the actual search result is on a page higher than 5 (the currently hardcoded limit).

The search could be additionally improved by searching only for tracks (and not artiststs and albums) when attempting to find streams, but that would require extending the bandcamp scraper module with this kind of functionality. Yet it shouldn't be too hard.

Notes:

When the name of a track was very simple, the actual search match could
be on a page higher than the configured maximum. This caused the search
to fail and the track wouldn't be played. The amount of search results
can be greatly reduced by simply adding the artist name to the query.
@nuki-chan
Copy link

nuki-chan bot commented Feb 11, 2024

Lines 18-34 in BandcampPlugin.ts

Oh, sweet innocent coder, Nuki spies a loop that's begging for a functional paradigm makeover! Right now, you're imperatively looping through pages as if we're back in 2005. Let's upgrade that for-loop to a modern, sleek, and asynchronous approach!

Here's what Nuki suggests:

  • Use Array.from to create a range of pages.
  • Map that range to promises using the Bandcamp search.
  • Use Promise.all to wait for all the pages, and then flatten the results.
  • Filter through the flattened array once to find all the tracks.
  • Stop as soon as you find a track - we can use find or even better, the sweet Array.prototype.some().

Such functional beauty, much wow! 😍 Now, bring out your refactoring spell!

Replace:

const limit = 5;
let tracks: BandcampSearchResult[];

const trackQuery: string = this.createTrackQuery(query);
const isMatchingTrack = this.createTrackMatcher(query);
for (let page = 0; page < limit; page++) {
  const searchResults = await Bandcamp.search(trackQuery, page);
  tracks = searchResults.filter(isMatchingTrack);
  if (tracks.length > 0) {
    break;
  }
}
return tracks;

With something more modern and functional, like:

const trackQuery: string = this.createTrackQuery(query);
const isMatchingTrack = this.createTrackMatcher(query);
const limit = 5;

const trackPromises = Array.from({ length: limit }, (_, page) =>
  Bandcamp.search(trackQuery, page)
);

const tracks = (await Promise.all(trackPromises))
  .flat()
  .filter(isMatchingTrack);

return tracks.length > 0 ? [tracks[0]] : [];

This way you're fetching all pages in parallel and then stopping after finding the first match. Efficient and elegant, like Nuki's coding style! ✨


General Comment

Nuki is utterly bamboozled by the amount of code duplication between BandcampPlugin.test.ts and BandcampPlugin.ts! You've got more redundancy here than an echo in a valley! 😆

Do consider abstracting common logic or, even better, use some mocks (nicely done with the spyOn by the way) so we don't see the same chunks of logic twice. Remember: Don't Repeat Yourself (unless you're telling someone how awesome you are, which Nuki fully endorses).

Keep the tests lean and mean, and the code clean! 🧼 (And always leave room for dessert 🍰!)

@sferra sferra changed the title Fix imprecise stream seaarch in the Bandcamp plugin Fix imprecise stream search in the Bandcamp plugin Feb 11, 2024
Copy link

codecov bot commented Feb 11, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0c7c69a) 71.58% compared to head (d175585) 71.60%.

Files Patch % Lines
packages/core/src/plugins/stream/BandcampPlugin.ts 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1549      +/-   ##
==========================================
+ Coverage   71.58%   71.60%   +0.02%     
==========================================
  Files         364      364              
  Lines        6707     6713       +6     
  Branches      479      479              
==========================================
+ Hits         4801     4807       +6     
  Misses       1512     1512              
  Partials      394      394              

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

@sferra
Copy link
Contributor Author

sferra commented Feb 11, 2024

Might fix #1236 and #1271.

Edit:
I checked, unfortunately it doesn't fix #1236.

@nukeop nukeop merged commit d01e199 into nukeop:master Feb 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants