alastair
404 while fetching by ID should not be a simple ResponseError
Currently when issuing get_releases_by_discid I get a ResponseError for any of the status codes 400, 404 and 411.
However, I do think a 404 is quite distinct from other ResponseErrors.
When checking for a discid I want to know if the disc ID is not found on the server or if there really was a response error.
That should either be a None as a return value or an Exception distinguishable from ResponseError (without having to check the cause or message). It can be a derived class, though.
Kind of related to #80. We need to think about the types of errors (5xx, 4xx) and why they are caused.
Maybe a NotFoundError and InvalidInputError or similar
Well, yes. "kind of" related to #80. Error code 400 is a real exceptional case though. Code 404 is a standard occurance when you try to check if an ID (that is normally valid) exists.
That is a bit different than with other mbids, since most mbids only exist because they were created in the MusicBrainz database. Disc IDs exists for every disc, indepently from their inclusion at musicbrainz.org. Same with ISRCs, PUIDs and echoprints.
NotFoundError sounds fine, but there are also some standard exceptions like LookupError or KeyError that would work.
None would still be a possibility. MB does return an error and no empty result, but consider:
>>> x = dict()
>>> x["blub"]
KeyError: 'blub'
>>> x.get("blub")
>>>
The last operation returns None.
Our functions is called get_...
This can't be changed without breaking any code, because users interested in the 404 needed to check for the cause, which I would not consider part of the API.
Deriving from ResponseError doesn't break code that is not interested in any 404 results and doesn't expect None as a result.
I personally would break code that doesn't check for None.
Users of "normal" mbids won't receive 404 in normal operation and users of ISRCs, disc IDs and echoprints really should have reported that as a bug.
https://github.com/JonnyJD/musicbrainz-isrcsubmit/commit/7474d2c0f86a0f5b3646fa601938bd076d60a8da
Gives a good example of a use case.
The check if response.get("disc") is not necessary at the moment, but would be when None is returned.
In that case the extra exception check is not necessary though.