Exceptions #10

Merged
merged 4 commits into from Jun 9, 2013

Projects

None yet

2 participants

Owner

Errors should never pass silently. I therefore suggest to raise an exception whenever SoCo would previously just return an error code/string.

Owner

Absolutely ;) We have been missing proper exceptions. I've looked through the code and I think it looks really good. A few things, suggestions or questions really, came to mind while reading it.

  1. The location of the exceptions. So we only have 2 of them right now, but maybe it would be worth it to move them to a separate file right from the start, also to help in keeping the size of core.py down.

  2. Doc strings. When I first read the code I remember thinking that the doc strings for the soco methods were quite verbose in the explanation of the error handling and that it would to somehow fix that when we introduced exceptions. I'm wondering if it would not actually be ok to replace the entire:

If an error occurs, we'll attempt to parse the error and raise a
SoCoException with the UPnP error code. If that fails, an
UnknownSoCoException with the raw response sent back from the Sonos
speaker will be raised.

with simply

Raises SoCoException or UnknownSoCoException upon errors.

and then add a thorough explanation in the exceptions classes instead (which people will be able to read about on ReadTheDocs). That would save us a lot of repeated "code" and I think still properly provide the information that people need.

  1. True return values and exceptions. I have been wondering actually if there is a convention about whether or not to return something like a True return value for a method whose only other possible outcome is to raise an exception if it fails. I mean, the return value is redundant, but I don't know whether it should be removed.

Just a few thought. Nice work and let me know what you think.
\Kenneth

Owner
  1. I've moved the exception classes to soco.exceptions in 4e8e761

  2. I've shortened the text to

Raises SoCoException (or a subclass) upon errors.

which is a bit more generic (cd41bac).

  1. I think those redundant return values can be removed. I don't know any common method in the stdlib that either returns True on success or raises an exception. Not explicitly returning anything (or rather: not guaranteeing any return values) leaves the option to return something useful later on.

So is there consensus to remove those return values?

Owner

I think the commits look good. Let's see where the other stand on the return values.

Owner

I've just removed the return True on success now ;)

@rahims, what do you think?

Owner

It would be great to get this committed. @rahims do you have any thoughts on the above?

On a related issues, maybe we should discuss what it takes to approve something for commit. I will open the discussion in a separate thread.

Owner

As per the new code acceptance criteria and @rahims comments on that, I think it should be safe to merge this. @stefankoegl will you do the honors?

@stefankoegl stefankoegl merged commit 5888fdd into SoCo:master Jun 9, 2013
Owner

I've rebased my changes onto master and pushed them to the main repository. Thanks :)

@stefankoegl stefankoegl deleted the stefankoegl:exceptions branch Jun 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment