Skip to content

Conversation

@palaviv
Copy link
Contributor

@palaviv palaviv commented Feb 12, 2016

I noticed that when I use the message fallback instead of the cython i get different Exception. On my way to fixing this I tried to improve the exceptions in general.

The branch does the following improvements:

  1. All exceptions inherit from MsgpackBaseException.
  2. The fallback and the cython throw the same exception type.
  3. Added new exception PackOverflowError for backwards compatibility.

…ckBaseException. cython and fallback throws same exceptions
@methane
Copy link
Member

methane commented Feb 12, 2016

All exceptions inherit from MsgpackBaseException.

I don't like it. Why base exception is required?

In [1]: import json

In [2]: json.loads("xyzzy")
JSONDecodeError: Expecting value: line 1 column 1 (char 0)

In [3]: json.JSONDecodeError.__mro__
Out[3]: (json.decoder.JSONDecodeError, ValueError, Exception, BaseException, object)

There are no BaseJsonException in json module.

@palaviv
Copy link
Contributor Author

palaviv commented Feb 12, 2016

I think it is easier for a user when there is a base exception if he wants to catch all exception that are related to the specific module. I think maybe changing it to MsgpackException from MsgpackBaseException will be better.
For example in requests all exceptions inherit from requests.exceptions.RequestException (http://docs.python-requests.org/en/master/user/quickstart/#errors-and-exceptions).

@methane
Copy link
Member

methane commented Feb 12, 2016

I think it is easier for a user when there is a base exception if he wants to catch all exception that are related to the specific module.

He should catch just Exception.

In case of requests module, there are many completely different reasons to fail to request: lookup, connect, HTTP error, timeout, etc...
In case of msgpack, I don't think there is enough reason to choose different API design from json.

@methane
Copy link
Member

methane commented Feb 12, 2016

All difference between msgpack module and json module should be coming from difference between msgpack and json.
If you have confidence about adding root exception is better, please add it to json module before msgpack module.

@palaviv
Copy link
Contributor Author

palaviv commented Feb 12, 2016

I thought the root exception will be something nice to add (I don't see any down side) but I can see why msgpack is more like json and less like requests.
I removed the MsgpackBaseException and the tests are working now.

x = -(2 ** 63)
assert unpackb(packb(x)) == x
with pytest.raises((OverflowError, ValueError)):
with pytest.raises(expected_exception):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this parameterize is too much.
Test only with most concrete exception type.

@methane methane mentioned this pull request Feb 14, 2016
@methane
Copy link
Member

methane commented Feb 14, 2016

I thought the root exception will be something nice to add (I don't see any down side)

More public names is significant downside.
It makes harder to maintainance and performance tuning.

@methane methane merged commit e15085d into msgpack:master Feb 14, 2016
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

Successfully merging this pull request may close these issues.

2 participants