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.

Sign up for GitHub

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

Jump to bottom

Improved output typing for lookup with inc parameter #826

Open
Haschikeks opened this issue Nov 20, 2023 · 5 comments · May be fixed by #836
Open

Improved output typing for lookup with inc parameter #826

Haschikeks opened this issue Nov 20, 2023 · 5 comments · May be fixed by #836
Assignees
@Haschikeks
Labels
improvement

Comments

@Haschikeks
Copy link
Collaborator

Haschikeks commented Nov 20, 2023 •
edited
Loading

Okay, so after experimenting a little bit we would need to do something like this

export interface ReleaseIncludes {
  labels: ILabel;
  recordings: IRecording;
}

and use it in the lookup functions like this:

public lookupRelease<T extends (keyof mb.RInc)[]>(releaseId: string, inc: T) {
  return this.lookupEntity<mb.IRelease & { [K in T[number]]: mb.RInc[K] }, keyof mb.RInc>('release', releaseId, inc);
}

const r = await this.lookupRelease("", ["labels"]);
const title = r.title;
const labels = r.labels.name;
// This throws an error
const recordings = r.recordings;

Originally posted by @Haschikeks in #504 (comment)
@Haschikeks
Copy link
Collaborator Author

I'll try and get a pull request, with updated lookup return types based on the inc paramter, ready till the end of the week.

@Borewit
Copy link
Owner

Borewit commented Nov 20, 2023

Do you want me to wait with a new release until you implemented this one?

@Haschikeks
Copy link
Collaborator Author

no, go ahead. This might take me a little bit longer.

Haschikeks pushed a commit that referenced this issue Nov 24, 2023
update return type of lookup based on the inc parameter;
f3cf4e6 
Closes #826
@Haschikeks
Copy link
Collaborator Author

Hey @Borewit

I've pushed to the connected branch. I noticed that some interfaces like IArtist already had some of the possible includes as part of there interface. I've removed some of them but I need to do some further investigation to check all the fields.

I also noticed to problem of different inc parameters not using the key in the json result. One example is

const release = await mbApi.lookup(
      "release",
      "478aaba4-9425-4a67-8951-a77739462df4",
      ["recordings"]
    );

This call results into the following json. But notice that there is no key for recordings, but instead its data is in the media key.

I'm currently not sure on how to handle those cases.

Details { "title": "Anomalie", "media": [ { "tracks": [ { "id": "3ff33b1b-aed6-44cc-a1be-4b50aab03dc1", "title": "Anomalie", "length": 189000, "position": 1, "recording": { "video": false, "disambiguation": "", "length": 189520, "title": "Anomalie", "id": "a985da67-a33c-499b-80ef-23f9046b83d5", "first-release-date": "2015-10-21" }, "number": "1" }, ], "position": 1, "track-offset": 0, "track-count": 10, "title": "", "format-id": "907a28d9-b3b2-3ef6-89a8-7b18d91d4794", "format": "Digital Media" } ], "status": "Official", "quality": "normal", "country": "FR", "packaging-id": null, "asin": null, "date": "2016-02-12", "packaging": null, "status-id": "4e304316-386d-3409-af2e-78857eec5cfe", "cover-art-archive": { "back": false, "count": 1, "darkened": false, "front": true, "artwork": true }, "id": "478aaba4-9425-4a67-8951-a77739462df4", "barcode": null, "text-representation": { "language": "fra", "script": "Latn" }, "disambiguation": "", "release-events": [ { "area": { "iso-3166-1-codes": [ "FR" ], "type-id": null, "type": null, "name": "France", "disambiguation": "", "sort-name": "France", "id": "08310658-51eb-3801-80de-5a0739207115" }, "date": "2016-02-12" } ] }

@Borewit
Copy link
Owner

Borewit commented Nov 24, 2023

I've pushed to the connected branch.

Maybe good to turn that into a draft PR, that makes it easier to discuss.

I noticed that some interfaces like IArtist already had some of the possible includes as part of there interface.

Indeed.

But notice that there is no key for recordings, but instead its data is in the media key.

Which is declared here:

musicbrainz-api/lib/musicbrainz.types.ts

Line 140 in 7bd63d8

recording: IRecording;

which should probably be made optional:

recording?: IRecording;

I honestly think that is more then enough. I strongly doubt if it is worth to derive the presence of those nested fields from the provided inclusion parameters.

I'm currently not sure on how to handle those cases.

Just keep those as optional as described above.

Maybe better to spend our effort in unit tests, which check if the MusicBrainz result is complaint with our typings. I bet we can shake some errors and incomplete definitions out that way.

@Haschikeks Haschikeks linked a pull request Nov 29, 2023 that will close this issue
Draft
@Haschikeks Haschikeks linked a pull request Nov 29, 2023 that will close this issue
update return type of lookup based on the inc parameter; #836
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Haschikeks Haschikeks

Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

update return type of lookup based on the inc parameter; Borewit/musicbrainz-api
2 participants
@Haschikeks @Borewit