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

Error handling practice #197

Open
Alejandro-Roldan opened this issue Oct 7, 2022 · 1 comment
Open

Error handling practice #197

Alejandro-Roldan opened this issue Oct 7, 2022 · 1 comment

Comments

@Alejandro-Roldan
Copy link

Hi!
Ive realized that the error handling of commands is a bit limited. I explain.
So for example if we try to do client.sticker_get("song", uri, tag) and the particular sticker doesnt exist in that uri we get a:

CommandError: [50@0] {sticker} no such sticker

But if the sticker database isnt active we get

CommandError: [5@0] {sticker} sticker database is disabled

Same error (a very clear different accompany text tho, nice) for two arguebly very different things

In a example code:

try:
    played = int(client.sticker_get("song", song["file"], "played"))
except CommandError:
    played = 0
try:
    client.sticker_set("song", song["file"], "played", played + 1)
except CommandError:
    pass

Against:

try:
    played = int(client.sticker_get("song", song["file"], "played"))
except NoStickerError:
    played = 0
except StickerDBError:
    pass
else:
    client.sticker_set("song", song["file"], "played", played + 1)

Dont give too much importance to the example itself, its just where i realized this, given the solution it doesnt make "much" difference; but i do belive it would be better practice to have clearer errors. Havent tried but I imagine similar things might occur with other commands.

Also it could be argued that if a sticker doesnt exist it could return None instead of error. That would make the code:

# At the start of the code
try:
    # A test to see that stickers db is activated
except StickerDBError:
    # whatever

# ...

played = client.sticker_get("song", song["file"], "played")
if played is None:
    played = 0
client.sticker_set("song", song["file"], "played", int(played) + 1)

But i gues thats more about what mpd itself wants to do, and it wouldnt be quite right to have a different behaviour here

Thanks for the amazing library! :D

@Mic92
Copy link
Owner

Mic92 commented Feb 18, 2023

Only problem though is that these types of errors are not really specified by the MPD protocol: https://mpd.readthedocs.io/en/latest/protocol.html#stickers
So different implementations i.e. mpd vs mopibity could do different things here.
However what you can do in your application is to use the errno included in the error to tell them apart:

>>> try: c.sticker_get("song", "foo", "played")
... except mpd.base.CommandError as e: print(e.errno)
... 
FailureResponseCode.NO_EXIST
>>> try: c.sticker_get("song", "foo", "played")
... except mpd.base.CommandError as e: print(e.errno)
... 
FailureResponseCode.UNKNOWN
>>> try: c.sticker_get("song", "foo", "played")
... except mpd.base.CommandError as e: print(e.msg)
... 
sticker database is disabled

Or you compare e.msg instead...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants