#6292 Refactor the initialization of ipalib.Parameter
Opened 7 years ago by mbabinsk. Modified 6 years ago

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.


master:

  • c3e3130 Tests: Fix failing test_ipalib/test_parameters

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

6 years ago

Login to comment on this ticket.

Metadata