args doesn't always have len()

[ahundt]

many functions check len(args) but args may not always support len().

Therefore I suggest the first line of each such function do something like the following on the first line:

if args is not None and hasattr(args, '__len__') and len(args):
    # current code here
else:
   raise TypeError('Unsupported <classname> constructor argument of type ' + str(type(args)))

Alternatively we could use **kwargs dictionary instead of args to avoid the problem entirely.

I think this safety is worth the overhead, and in the case it isn't the pure c class should most likely be used or a c++ version of your function should be implemented anyway. What do you think?

[haudren]

In which case does args have no len ? According to the Python (possibly incomplete) docs

If the form “*identifier” is present, it is initialized to a tuple receiving any excess positional parameters, defaulting to the empty tuple. If the form “*identifier” is present, it is initialized to a new dictionary receiving any excess keyword arguments, defaulting to a new empty dictionary. Parameters after “” or “*identifier” are keyword-only parameters and may only be passed used keyword arguments.

AFAIK, a tuple always has a len.

[ahundt]

AFAIK, a tuple always has a len.

@haudren The user might not call the constructor with a tuple:

$ import eigen as e
$ e.Quaterniond(1)
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/xonsh/__amalgam__.py", line 15472, in default
    run_compiled_code(code, self.ctx, None, 'single')
  File "/usr/local/lib/python3.6/site-packages/xonsh/__amalgam__.py", line 3634, in run_compiled_code
    func(code, glb, loc)
  File "/usr/local/lib/python3.6/site-packages/xontrib/vox_tabcomplete.xsh", line 1, in <module>
    def _vox_completer(prefix, line, begidx, endidx, ctx):
  File "eigen/eigen.pyx", line 2593, in eigen.eigen.Quaterniond.__cinit__
TypeError: <class 'int'>

What I'm saying is that we could provide a better error message than the above. 👍 Does that sound reasonable?

[ahundt]

For some reason in my original post I decided to print instead of raise TypeError(...), I've corrected that.

[gergondet]

@ahundt This has nothing to do with the fact that args is not a tuple (it is)

The culprit is actually

"Unsupported argument types passed to Quaterniond ctor: ".join([str(type(x)) for x in args]

which I didn't catch in the review but should be:

raise TypeError("Unsupported argument types passed to Quaterniond ctor: " + ", ".join([str(type(x)) for x in args] ))

which will give you a much better error message:

TypeError: Unsupported argument types passed to Quaterniond ctor: <type 'int'>

[haudren]

Thanks Pierre, I couldn't see the bug :)

[haudren]

Fixed by c2a38ef

The output is now:

Traceback (most recent call last):
  File "test.py", line 3, in <module>
    e.Quaterniond(1)
  File "eigen/eigen.pyx", line 2593, in eigen.eigen.Quaterniond.__cinit__ (eigen/eigen.cpp:86944)
TypeError: Unsupported argument types passed to Quaterniond ctor: <type 'int'>

[ahundt]

@haudren Closed a bit too early! There are a lot more instances of similar strings that need to be fixed, all the Matrix classes, AngleAxis, etc.

[gergondet]

I forgot to post a message here but I fixed the other in d095fb1

[ahundt]

sweet, sorry for creating the bug in the first place. I think now that it works this error message is quite helpful, thanks for the fixes! :-)

I'm not using every API, but for the ones I do use I estimate I've now made ~18 million calls and they're holding up well.