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

Simplify lookup, browse and search functions #823

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Nov 15, 2023

Remove specific search function in favor of a single typed function:

  • mbApi.browseArtist(query) with mbApi.browse('artist', query)
  • mbApi.lookupArtist('ab2528d9-719f-4261-8098-21849222a0f2') with mbApi.lookup('artist', 'ab2528d9-719f-4261-8098-21849222a0f2')
  • mbApi.searchArtist({query: 'Stromae'}) with mbApi.search('artist', {query: 'Stromae'})

Resolves #822

@Borewit Borewit requested a review from Haschikeks November 15, 2023 21:07
@Borewit Borewit self-assigned this Nov 15, 2023
@Borewit Borewit changed the title Simplify lookup function Simplify lookup, browse and search functions Nov 18, 2023
lib/musicbrainz-api.ts Fixed Show fixed Hide fixed
@Borewit Borewit force-pushed the simplify-lookup branch 2 times, most recently from 44413a8 to 664b49b Compare November 18, 2023 12:47
@Borewit
Copy link
Owner Author

Borewit commented Nov 18, 2023

If you are happy with this one @Haschikeks , I like to merge it.

lib/musicbrainz-api.ts Outdated Show resolved Hide resolved
@Borewit Borewit force-pushed the simplify-lookup branch 2 times, most recently from f00f011 to 7e73e3b Compare November 19, 2023 12:02
Copy link
Collaborator

@Haschikeks Haschikeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
But we should keep in mind, that this is a breaking API change.

@Borewit
Copy link
Owner Author

Borewit commented Nov 20, 2023

But we should keep in mind, that this is a breaking API change.

Very true.
I will write add a warning in the release notes.
We are still releasing as 0.x.x versions, which means the API is subject to change.
Although I think this one is received well, we should soon release 1.0.0.

Thanks for your review @Haschikeks, much appreciated.

@Borewit Borewit merged commit 8c775ee into master Nov 20, 2023
@Borewit Borewit deleted the simplify-lookup branch November 20, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG Property 'recordings' does not exist on type 'ISearchResult'.
2 participants