Replacing ValueError with InvalidArgumentError in google.gax.errors.create_error(). #196
Conversation
…reate_error(). This new class inherits from both GaxError and ValueError to keep from breaking existing clients. By wrapping an `INVALID_ARGUMENT` in a `ValueError`, the impl. was losing information by "stringify"-ing the wrapped exception. See #132 for change that introduced the `ValueError` behavior.
dhermes
changed the title
|
@lukesneeringer PTAL |
|
Waiting on CI, then happy to merge. |
| status_code = config.exc_to_code(cause) | ||
| status_name = config.NAME_STATUS_CODES.get(status_code) | ||
| if status_name == 'INVALID_ARGUMENT': | ||
| return InvalidArgumentError(msg, cause=cause) |
There was a problem hiding this comment.
@lukesneeringer @jonparrott It's worth noting here that, str(exc) has still changed. We went from:
>>> msg = 'RPC failed'
>>> exc = ValueError('{}: {}'.format(msg, cause))
>>> str(exc)
'RPC failed: <_Rendezvous of RPC that terminated with (StatusCode.INVALID_ARGUMENT, Resource name invalid.)>'
>>> exc.args
('RPC failed: <_Rendezvous of RPC that terminated with (StatusCode.INVALID_ARGUMENT, Resource name invalid.)>',)
>>> raise exc
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: RPC failed: <_Rendezvous of RPC that terminated with (StatusCode.INVALID_ARGUMENT, Resource name invalid.)>to:
>>> exc = InvalidArgumentError(msg, cause=cause)
>>> str(exc)
'InvalidArgumentError(RPC failed, caused by <_Rendezvous of RPC that terminated with (StatusCode.INVALID_ARGUMENT, Resource name invalid.)>)'
>>> exc.args
('RPC failed',)
>>> raise exc
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
google.gax.errors.InvalidArgumentError: InvalidArgumentError(RPC failed, caused by <_Rendezvous of RPC that terminated with (StatusCode.INVALID_ARGUMENT, Resource name invalid.)>)Aside, the setup is:
>>> import grpc
>>> from grpc import _channel
>>>
>>> status_code = grpc.StatusCode.INVALID_ARGUMENT
>>> details = 'Resource name invalid.'
>>> exc_state = _channel._RPCState((), None, None, status_code, details)
>>> cause = _channel._Rendezvous(exc_state, None, None, None)
There was a problem hiding this comment.
One potential bad thing about this is that there is nothing to tell the user to check cause.
Perhaps it would be worth defining a __str__ that gives a more useful message? (It does not have to be the same message.)
There was a problem hiding this comment.
@lukesneeringer Are you proposing a change to GaxError.__str__ or something else?
There was a problem hiding this comment.
I was proposing InvalidArgumentError.__str__ (but I am not married to that). If GaxError has a cause attribute (I think it does) than it is the better choice. Then you just have to make sure that GaxError is before ValueError in your MRO (so, flip what you have now).
There was a problem hiding this comment.
@lukesneeringer Actually, un-MERP. Double check my comment above. It has the "correct" representation:
>>> str(exc)
'InvalidArgumentError(RPC failed, caused by <_Rendezvous of RPC that terminated with (StatusCode.INVALID_ARGUMENT, Resource name invalid.)>)'
There was a problem hiding this comment.
I am no longer tracking the state of this. Is there an outstanding question/issue or should I merge?
There was a problem hiding this comment.
I am just unsure what you actually wanted.
|
@lukesneeringer CI is green, but LMK what you think about the str/repr/stacktrace changes |
|
@lukesneeringer Can we merge / resolve? |
|
@lukesneeringer @tseaver @jonparrott Let's get this thing to the finish line? |
|
I don't have merge privileges here. |
This new class inherits from both GaxError and ValueError to keep from breaking existing clients.
By wrapping an
INVALID_ARGUMENTin aValueError, the impl. was losing information by "stringify"-ing the wrapped exception.See #132 for change that introduced the
ValueErrorbehavior.