Support the track/list_by_mbid API route #57
Conversation
jbosboom
changed the title
There was a problem hiding this comment.
This looks awesome! My sincerest apologies for letting this PR slip through the cracks—I don't usually make a habit out of letting things go for months at a time. 😳
This seems good already. One design thing, however, is that I generally think it can be pretty confusing when functions can substantially change their output format (i.e., return type) depending on the values of the arguments. The current implementation does this in two ways (not your fault—this is the way the underlying API works): batch vs. single, and whether to include disabled MBIDs. So what do you think about moving the core functionality to a helper function and providing wrappers with consistent return types?
One way to do this would be to have a batch function and single-ID function. That seems nice but it doesn't solve the disabledness problem, which is trickier. Maybe we should just live with that one.
| message = response['error']['message'] | ||
| except KeyError: | ||
| raise WebServiceError("response: {0}".format(response)) | ||
| raise WebServiceError("error {0}: {1}".format(code, message)) |
There was a problem hiding this comment.
Indeed, as you mentioned, maybe this can be factored out into a function to avoid duplication. The two options are for it to go into _api_request or into a new, separate function. The former would be more appropriate if we only ever use _api_request this way; the latter might be better if some calls to _api_request need to process stuff differently.
| mbids = response['mbids'] | ||
| if disabled: | ||
| return {m['mbid']: | ||
| ([x['id'] for x in m['tracks'] if 'disabled' not in x or not x['disabled']], |
There was a problem hiding this comment.
This bit:
'disabled' not in x or not x['disabled']is equivalent to this slightly shorter expression:
not x.get('disabled')
Implements #56.
There's not much structure to the returned data so this function parses the response itself instead of having a separate parse function.
I copied the error-raising code from submit. Maybe that should be folded into
_api_request?AcoustIDs can be disabled (soft-deleted). When the caller asks to include the disabled ids with
disabled=Truewe return a pair of lists instead of a list of(id, disabled)pairs. Neither option feels great, but I expect anyone who wants the disabled ids to treat them differently from the enabled ones, so I went with the pair of lists. I am willing to change this if you think list-of-pairs is better.I'm not a "native" Python developer so bikeshedding and style nits are appreciated.