zeromq / pyre

Python port of Zyre
GNU Lesser General Public License v3.0
121 stars 52 forks source link

`ctx` is a defined function parameter that's always overwritten, i.e. never in `kwargs` #131

Open jimdewees opened 6 years ago

jimdewees commented 6 years ago

https://github.com/zeromq/pyre/blob/b2a3c9b4d5c7fd08b011e0464b18db997f25df5f/pyre/pyre.py#L41

>>> class C(object):
...     def __init__(self, ctx=None, *args, **kwargs):
...         print(ctx, kwargs, kwargs.get('ctx'))
...
>>> try:
...     c1 = C('this', ctx='that')
... except TypeError as err:
...     print(err)
...
__init__() got multiple values for argument 'ctx'
>>> try:
...     c2 = C(ctx='this', **{'ctx': 'that'})
... except TypeError as err:
...     print(err)
...
type object got multiple values for keyword argument 'ctx'
>>> c3 = C('this', _ctx='that')
this {'_ctx': 'that'} None
>>> c4 = C(ctx='this is not in kwargs')
this is not in kwargs {} None
>>> c5 = C(**{'ctx': 'neither is this'})
neither is this {} None
sphaero commented 6 years ago

so what are you suggesting?

jimdewees commented 5 years ago

The docstring for zmq.Context.instance provides a good implementation example.

sphaero commented 5 years ago

You're not very verbose. Are you suggesting that:

ctx = kwargs.get('ctx')
if ctx == None:
    ctx = zmq.Context()
self._ctx = ctx

should be reduced to

self._ctx = ctx or zmq.Context()
fieldOfView commented 5 years ago

That would fix the issue that you are trying to get ctx from the kwargs dict, whereas it is defined as a proper keyword argument in the __init__ method definition.