BIND 10 #1258: "preserve order" option for Message::fromWire()
BIND 10 Development
do-not-reply at isc.org
Fri Sep 30 18:29:52 UTC 2011
#1258: "preserve order" option for Message::fromWire()
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
jinmei | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20111011
blocker | Resolution:
Component: | Sensitive: 0
libdns++ | Sub-Project: DNS
Keywords: | Estimated Difficulty: 0
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:5 jelte]:
Thanks for the prompt review.
> I've pushed a trivial comment change in message.h (and my editor removed
a few
> spaces in that file)
Looks good, thanks.
> Code looks good; just one small comment:
>
> message_python.cc:688: Shouldn't we just let the normal python error be
> propagated here? It should also specify which types are wanted, albeit
only
> from the second call. Otherwise, I'd propose to at least add which types
we
> expect to the error message (just 'invalid arguments' is true but not
extremely
> helpful).
Hmm, I added explicit PyErr_SetString() just following the convention
adopted in many other such cases of our binding without considering
the implications much. So I checked the behavior without removing
PyErr_SetString(), and I found it possibly more confusing in some cases.
For example, from_wire(1, 1) would result in:
{{{
TypeError: function takes exactly 1 argument (2 given)
}}}
This is rather confusing as this method should actually accept 2
arguments.
On the other hand, I agree that just saying 'invalid arguments' isn't
helpful either.
So, I chose to keep our own PyErr_SetString() and tried to make the
error string more helpful. Please check.
--
Ticket URL: <http://bind10.isc.org/ticket/1258#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list