Skip to content

Conversation

@jagerman
Copy link
Member

The changes the error message for invalid arguments to display Type (= strvalue) instead of just strvalue for the given argument list.

Fixes #517.

@dean0x7d
Copy link
Member

What about using py::repr? It would show type info in non-trivial cases, but leave simple things simple and avoid type duplication like NoneType (= None) and Pet (= <m.Pet object at 0>).

@jagerman
Copy link
Member Author

@dean0x7d - you mean simply instead of the type + value pair? I thought it might be nice to have both type and value, even if it is redundant sometimes (like None, Pet).

@jagerman
Copy link
Member Author

jagerman commented Nov 21, 2016

But yeah, perhaps just repr instead of the value would be better.

@dean0x7d
Copy link
Member

Yeah, just repr where it was str originally. Seems like exactly what repr was intended for.

@jagerman
Copy link
Member Author

jagerman commented Nov 21, 2016

While it's nicer than str, I'm on the fence as to whether it's enough. The only practical change it makes in the test scripts is adding quotes around the arguments of 'no', 'such', 'constructor'. On the other hand, the list of overloads directly lists the types, so including the type name as well might be more informative (e.g. including both type.__name__ and repr(value)).

@jagerman
Copy link
Member Author

@pschella - thoughts?

@dean0x7d
Copy link
Member

In most cases repr already contains the type:

>>> import datetime
>>> now = datetime.datetime.now()
>>> str(now)
2016-11-21 21:32:47.664461
>>> repr(now)
datetime.datetime(2016, 11, 21, 21, 32, 47, 664461)

repr is quite clear about the type and as a nice bonus we can use eval:

>>> then = eval(repr(now))
>>> str(then)
2016-11-21 21:32:47.664461

The examples in the tests are mostly builtin types which can be trivially reconstructed. That's why str(1) == repr(1) but for strings the quotes are added:

>>> s = "hello"
>>> str(s)
hello
>>> repr(s)
'hello'
>>> eval(str(s))
NameError: name 'hello' is not defined
>>> eval(repr(s))
hello

This gives more informative output, often including the type (or at
least some hint about the type).
@jagerman jagerman force-pushed the include-type-in-badargs-error branch from be6f5e9 to f0ee877 Compare November 21, 2016 20:41
@jagerman
Copy link
Member Author

jagerman commented Nov 21, 2016

Okay, I'm convinced :)

Updated to just switch to repr (I also ditched the static_cast because it seems unnecessary--I think not required since #450).

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.

3 participants