feat(qobuz): Implement Qobuz provider#206
Conversation
|
~~Tests seem to fail with a server error 500 for whatever reason, but lookups are running fine outside of tests. Perhaps the test command I'm using is incorrect: Edit: seems the environment variables aren't loading properly during the test~~ Forgot to specify .env path :p
|
|
Just realized eti isn't included in track titles |
There was a problem hiding this comment.
Thank you very much, this is looking good already!
I've got a couple of comments nevertheless. These are only based on reading the code, I will come back once I've tested the provider with a couple of releases (but that will have to wait until I'm done with my Discogs provider testing).
P.S. The formatter doesn't like your API interface definitions, please fix with deno fmt. You can locally check that CI (except tests) passes by running deno task ok.
providers/Qobuz/mod.ts
Outdated
|
|
||
| readonly supportedUrls = new URLPattern({ | ||
| hostname: '(play|www|open).qobuz.com', | ||
| pathname: '/:region?/:type(artist|album|track|interpreter|label)/:slug?/:id', |
There was a problem hiding this comment.
We can't call the combination of country and language code :region here, it will cause problems for other providers which only expect a country code as region. The simplest solution would be to call the group :locale and be done with it, but this doesn't provide much value.
Alternatively we can extract two named groups :region and :language, which makes the region usable for other providers, but requires us to define a mapping between regions and default languages (for incoming regions from other providers or the web input where we don't know the language).
| "paid streaming", | ||
| "paid download", | ||
| ], | ||
| url: "https://www.qobuz.com/fr-fr/album/suspend-my-belief-ivycomb-stephanafro/rjrikcvbggy0b", |
There was a problem hiding this comment.
I'm not very familiar with Qobuz URL formats and which one is accepted on MB.
I only noticed that you are seeding the localized www URL here while the provider tests consider the non-localized open URLs without slug as canonical. Is there a reason for that discrepancy?
There was a problem hiding this comment.
All qobuz links are currently accepted on MB since there's no cleanup for them. The old www.qobuz links are the only ones displayed in the sidebar currently. I personally prefer the open.qobuz links for a couple reasons, so I'll likely switch to using those.
kellnerd
left a comment
kellnerd
left a comment
There was a problem hiding this comment.
deno test --allow-net --allow-write --allow-read --allow-env ./providers/Qobuz/mod.test.ts -- --download --update
Quick tips:
- You can abbreviate the allow flags with
-NWRE. - Usually you want either the
--downloador the--updateflag (or none of them most of the time). --downloadshould be rarely enabled after all the testdata has been cached once because responses often cause irrelevant noise for git because of unstable sort order (like your Qobuz search response changing with every commit :/) or irrelevant properties (like popularity) changing their values.- You should only
--updatethe snapshots if there was an intended change that caused an assertion error, otherwise it will always update and swallow unintended breaking changes.
| }], | ||
| externalLinks: [{ | ||
| url: rawRelease.url, | ||
| types: this.provider.getLinkTypesForEntity(), |
There was a problem hiding this comment.
Is there a reason why you are not using the streamable and downloadable properties here?
P.S. I thought this was part of my first review already, but apparently I forgot to hit the "Add review comment" button and GH remembered it until now 🤔
2 participants
Closes #189