The logic which merges passed in attributes with the defaults in the Parameter constructor is needlessly complex and fragile. Especially 'required' and 'multivalue' are prone to cause subtle errors when set to None as is demonstrated by the following failing test case:
<snip> o = Subclass('my_param') # Test with no **kw: for (key, kind, default) in o.kwargs: # Test with a type invalid for all: value = object() kw = {key: value} e = raises(TypeError, Subclass, 'my_param', **kw) if kind is callable: assert str(e) == CALLABLE_ERROR % (key, value, type(value)) else: assert str(e) == TYPE_ERROR % (key, kind, value, type(value)) # Test with None: kw = {key: None} > Subclass('my_param', **kw) ipatests/test_ipalib/test_parameters.py:265: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <[AttributeError("'Subclass' object has no attribute 'param_spec'") raised in repr()] SafeRepr object at 0x7f2736a77f38> name = 'my_param', rules = (), kw = {'multivalue': False} kw_from_spec = {'multivalue': False, 'required': True}, df = None key = 'default', kind = <type 'NoneType'>, default = None, value = None def __init__(self, name, *rules, **kw): # Merge in kw from parse_param_spec(): (name, kw_from_spec) = parse_param_spec(name) check_name(name) if not 'required' in kw: kw['required'] = kw_from_spec['required'] if not 'multivalue' in kw: kw['multivalue'] = kw_from_spec['multivalue'] # Add 'default' to self.kwargs if kw.get('multivalue', True): self.kwargs += (('default', tuple, None),) else: self.kwargs += (('default', self.type, None),) # Wrap 'default_from' in a DefaultFrom if not already: df = kw.get('default_from') if callable(df) and not isinstance(df, DefaultFrom): kw['default_from'] = DefaultFrom(df) # Perform type validation on kw: for (key, kind, default) in self.kwargs: value = kw.get(key) if value is not None: if kind in (tuple, frozenset): if type(value) in (list, tuple, set, frozenset): value = kind(value) elif type(value) is str: value = kind([value]) if ( type(kind) is type and not isinstance(value, kind) or type(kind) is tuple and not isinstance(value, kind) ): raise TypeError( TYPE_ERROR % (key, kind, value, type(value)) ) elif kind is callable and not callable(value): raise TypeError( CALLABLE_ERROR % (key, value, type(value)) ) kw[key] = value else: kw.pop(key, None) # We keep these values to use in __repr__(): > if kw['required']: E KeyError: 'required' ipalib/parameters.py:479: KeyError
We should re-write the logic so that it merges default and user-defined values robustly and can present them in multiple views (unified, defaults, and parameters from __init__) in a manner similar to the Python 3 Chainmap.
__init__
master:
Just partial fix, it needs much more, leaving ticket opened
Metadata Update from @mbabinsk: - Issue assigned to someone - Issue set to the milestone: Future Releases
Log in to comment on this ticket.