Get multiple recordings #209
Conversation
| ) | ||
|
|
||
| @bp_dataset_eval.route("/bulk-recordings/<recording_ids>", methods=["GET"]) | ||
| def bulk_recordings(recording_ids): |
There was a problem hiding this comment.
I would suggest to reuse existing endpoint by providing an option to specify list of IDs as an argument. I don't like the trend of creating more and more endpoints that provide multiple ways to get the same data. There should be one place to get a particular entity.
Think about how people are going to use the API, not just a client that you are building. It's hard to change things in the API once they are there.
There was a problem hiding this comment.
yeah, good idea. This is why I linked this ticket with http://tickets.musicbrainz.org/browse/AB-21 in jira. Let's add this functionality to the standard low-level and high-level endpoints in core.py.
There was a problem hiding this comment.
So kind of like an existing endpoint, but instead of having ID as a part of URL just have ids argument.
There was a problem hiding this comment.
Okay will incorporate that into the core API endpoint. @alastair what are your views about putting the recording ids as a part of the url or as a parameter?
kartikgupta0909
force-pushed
the
Get-Multiple-Recordings
branch
from
1fa2c22
to
7bcfafc
Compare
|
I have updated this PR with the changes suggested in the comments and the ones discussed on the irc.
|
| if len(recordings) > 200: | ||
| raise webserver.views.api.exceptions.APIBadRequest("More than 200 recordings not allowed per request") | ||
| for recording in recordings: | ||
| recording = str(recording).split(":") |
There was a problem hiding this comment.
So specifying an offset is mandatory? I don't think this is a good idea.
There was a problem hiding this comment.
No its not, Its optional. We are checking first if there are two parameters (id and an offset), if there is no offset we set a default offset of 0.
There was a problem hiding this comment.
Ah, good. I see the comparison below now.
kartikgupta0909
force-pushed
the
Get-Multiple-Recordings
branch
from
7bcfafc
to
1ee91c2
Compare
Rename the method and endpoint to refer to low-level, like the existing GET methods Catch <0 offsets If the same request is duplicated in the query, only perform the request once
also fix embarrassing spelling mistake. valilidate?
|
This is merged and released! |
This is related to http://tickets.musicbrainz.org/browse/AB-147