From a53d4c640109d638b7929b9399156631a881a727 Mon Sep 17 00:00:00 2001 From: Marc Skov Madsen Date: Sun, 26 Mar 2023 07:36:37 +0200 Subject: [PATCH 1/8] proof of concept for required --- .gitignore | 2 ++ param/parameterized.py | 18 +++++++++++++++-- tests/API1/testparameterizedobject.py | 28 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index f7cf9ccae..9b7e44d69 100644 --- a/.gitignore +++ b/.gitignore @@ -26,3 +26,5 @@ coverage.xml # autover param/.version +.venv/ +script.* diff --git a/param/parameterized.py b/param/parameterized.py index 607e01f94..e7e82ce54 100644 --- a/param/parameterized.py +++ b/param/parameterized.py @@ -970,7 +970,7 @@ class Foo(Bar): __slots__ = ['name', '_internal_name', 'default', 'doc', 'precedence', 'instantiate', 'constant', 'readonly', 'pickle_default_value', 'allow_None', 'per_instance', - 'watchers', 'owner', '_label'] + 'watchers', 'owner', '_label', "required"] # Note: When initially created, a Parameter does not know which # Parameterized class owns it, nor does it know its names @@ -983,7 +983,7 @@ class Foo(Bar): def __init__(self,default=None, doc=None, label=None, precedence=None, # pylint: disable-msg=R0913 instantiate=False, constant=False, readonly=False, pickle_default_value=True, allow_None=False, - per_instance=True): + per_instance=True, required=False): """Initialize a new Parameter object and store the supplied attributes: @@ -1051,6 +1051,8 @@ def __init__(self,default=None, doc=None, label=None, precedence=None, # pylint allowed. If the default value is defined as None, allow_None is set to True automatically. + required: If True a value must be provided on instantiation. + default, doc, and precedence all default to None, which allows inheritance of Parameter slots (attributes) from the owning-class' class hierarchy (see ParameterizedMetaclass). @@ -1070,6 +1072,7 @@ class hierarchy (see ParameterizedMetaclass). self.allow_None = (default is None or allow_None) self.watchers = {} self.per_instance = per_instance + self.required = required @classmethod def serialize(cls, value): @@ -1601,6 +1604,16 @@ def _generate_name(self_): self = self_.param.self self.param._set_name('%s%05d' % (self.__class__.__name__ ,object_count)) + @as_uninitialized + def _validate_params(self_, **params): + self = self_.param.self + params_to_instantiate = self.__class__.param._parameters + + for p,v in params_to_instantiate.items(): + if v.required and not v.name in params: + message = f"__init__() missing 1 required positional argument: '{v.name}'" + raise TypeError(message) + @as_uninitialized def _setup_params(self_,**params): @@ -3170,6 +3183,7 @@ def __init__(self, **params): self._dynamic_watchers = defaultdict(list) self.param._generate_name() + self.param._validate_params(**params) self.param._setup_params(**params) object_count += 1 diff --git a/tests/API1/testparameterizedobject.py b/tests/API1/testparameterizedobject.py index 487e1894c..27e9db52d 100644 --- a/tests/API1/testparameterizedobject.py +++ b/tests/API1/testparameterizedobject.py @@ -813,3 +813,31 @@ class B(A): # Should be 2? # https://github.com/holoviz/param/issues/718 assert B.p == 1 + +@pytest.mark.parametrize(["default", "value"], [ + (None,"v"), ("d", "v") +]) +def test_constant_parameter_not_raises(default, value): + """Test that you can make a parameter required and it will not raise + an error if provided.""" + class TestConstant(param.Parameterized): + value = param.Parameter(default=default, required=True) + + po = TestConstant(value=value) + assert po.value==value + +import re + +@pytest.mark.parametrize("default", [None, "d"]) +def test_constant_parameter_raises(default): + """Test that you can make a parameter required and it will raise + an error if not provided.""" + class TestConstant(param.Parameterized): + value = param.Parameter(default=default, required=True) + + # + with pytest.raises( + TypeError, + match=re.escape(r"__init__() missing 1 required positional argument: 'value'"), + ) as ex: + TestConstant() From 2c8608e4155f47cf42cf29a572bf27aa4fce5f40 Mon Sep 17 00:00:00 2001 From: Marc Skov Madsen Date: Sun, 26 Mar 2023 07:42:08 +0200 Subject: [PATCH 2/8] fix flake errors --- param/parameterized.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/param/parameterized.py b/param/parameterized.py index e7e82ce54..b5641e50e 100644 --- a/param/parameterized.py +++ b/param/parameterized.py @@ -1608,12 +1608,12 @@ def _generate_name(self_): def _validate_params(self_, **params): self = self_.param.self params_to_instantiate = self.__class__.param._parameters - + for p,v in params_to_instantiate.items(): - if v.required and not v.name in params: + if v.required and v.name not in params: message = f"__init__() missing 1 required positional argument: '{v.name}'" raise TypeError(message) - + @as_uninitialized def _setup_params(self_,**params): From a933f4524f9e5a86f708ea7703f4ac63ad0a3bf1 Mon Sep 17 00:00:00 2001 From: Marc Skov Madsen Date: Sun, 26 Mar 2023 07:47:14 +0200 Subject: [PATCH 3/8] fix import errors --- tests/API1/testparameterizedobject.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/API1/testparameterizedobject.py b/tests/API1/testparameterizedobject.py index 27e9db52d..c13653a19 100644 --- a/tests/API1/testparameterizedobject.py +++ b/tests/API1/testparameterizedobject.py @@ -4,6 +4,7 @@ import param import numbergen +import re from . import API1TestCase from .utils import MockLoggingHandler @@ -826,8 +827,6 @@ class TestConstant(param.Parameterized): po = TestConstant(value=value) assert po.value==value -import re - @pytest.mark.parametrize("default", [None, "d"]) def test_constant_parameter_raises(default): """Test that you can make a parameter required and it will raise From f97aa2a5e9653884030829982570b24f736de12b Mon Sep 17 00:00:00 2001 From: maximlt Date: Sat, 29 Apr 2023 18:55:44 +0200 Subject: [PATCH 4/8] extend a test --- tests/utils.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/utils.py b/tests/utils.py index 13771a4cb..7c0160e28 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -93,3 +93,5 @@ def check_defaults(parameter, label, skip=[]): assert parameter.per_instance is True if 'label' not in skip: assert parameter.label == label + if 'required' not in skip: + assert parameter.required is False From bbccbdcf5d466f6960e90acdf2fbfc14c40ead63 Mon Sep 17 00:00:00 2001 From: maximlt Date: Sat, 29 Apr 2023 18:56:34 +0200 Subject: [PATCH 5/8] change quotes --- param/parameterized.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/param/parameterized.py b/param/parameterized.py index 7361fcc2e..66947c101 100644 --- a/param/parameterized.py +++ b/param/parameterized.py @@ -986,7 +986,7 @@ class Foo(Bar): __slots__ = ['name', '_internal_name', 'default', 'doc', 'precedence', 'instantiate', 'constant', 'readonly', 'pickle_default_value', 'allow_None', 'per_instance', - 'watchers', 'owner', '_label', "required"] + 'watchers', 'owner', '_label', 'required'] # Note: When initially created, a Parameter does not know which # Parameterized class owns it, nor does it know its names From 4af6505e25acecf1988571bfdeb912b80c7c4527 Mon Sep 17 00:00:00 2001 From: maximlt Date: Mon, 1 May 2023 22:41:59 +0200 Subject: [PATCH 6/8] parse the right list of parameters and reformat error --- param/parameterized.py | 43 +++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/param/parameterized.py b/param/parameterized.py index 66947c101..5fcc0cd13 100644 --- a/param/parameterized.py +++ b/param/parameterized.py @@ -1074,7 +1074,8 @@ def __init__(self, default=Undefined, doc=Undefined, # pylint: disable-msg=R0913 allowed. If the default value is defined as None, allow_None is set to True automatically. - required: If True a value must be provided on instantiation. + required: If True a value must be provided on instantiation of a + Parameterized object. default, doc, and precedence all default to None, which allows inheritance of Parameter slots (attributes) from the owning-class' @@ -1657,15 +1658,35 @@ def _generate_name(self_): self.param._set_name('%s%05d' % (self.__class__.__name__ ,object_count)) @as_uninitialized - def _validate_params(self_, **params): - self = self_.param.self - params_to_instantiate = self.__class__.param._parameters - - for p,v in params_to_instantiate.items(): - if v.required and v.name not in params: - message = f"__init__() missing 1 required positional argument: '{v.name}'" - raise TypeError(message) - + def _check_required(self_, **params): + cls = self_.param.cls + + # Map of all the class parameters + parameters = {} + for class_ in classlist(cls): + for name, val in class_.__dict__.items(): + if isinstance(val, Parameter): + parameters[name] = val + # Find what Parameters are required but were not passed to the + # constructor. + missing = [ + pname + for pname, p in parameters.items() + if p.required and pname not in params + ] + # Format the error + if missing: + missing = [f'{s!r}' for s in missing] + if len(missing) > 1: + kw = ', '.join(missing[:-1]) + kw = kw + f'{"," if len(missing) > 2 else ""} and {missing[-1]}' + else: + kw = missing[0] + message = ( + f"{cls.name}.__init__() missing {len(missing)} required keyword-only " + f"argument{'s' if len(missing) >1 else ''}: {kw}" + ) + raise TypeError(message) @as_uninitialized def _setup_params(self_,**params): @@ -3218,7 +3239,7 @@ def __init__(self, **params): self._dynamic_watchers = defaultdict(list) self.param._generate_name() - self.param._validate_params(**params) + self.param._check_required(**params) self.param._setup_params(**params) object_count += 1 From 090584e0d59331c454d9fb73058a7e7f6a38b32a Mon Sep 17 00:00:00 2001 From: maximlt Date: Mon, 1 May 2023 22:42:07 +0200 Subject: [PATCH 7/8] add and update tests --- tests/testparameterizedobject.py | 86 ++++++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 10 deletions(-) diff --git a/tests/testparameterizedobject.py b/tests/testparameterizedobject.py index 2f47fa7b9..2794ac754 100644 --- a/tests/testparameterizedobject.py +++ b/tests/testparameterizedobject.py @@ -827,28 +827,94 @@ class B(A): @pytest.mark.parametrize(["default", "value"], [ (None,"v"), ("d", "v") ]) -def test_constant_parameter_not_raises(default, value): +def test_required(default, value): """Test that you can make a parameter required and it will not raise an error if provided.""" - class TestConstant(param.Parameterized): - value = param.Parameter(default=default, required=True) + class P(param.Parameterized): + value1 = param.Parameter(default=default, required=True) + value2 = param.Parameter(default=default, required=True) + notrequired = param.Parameter(default=default, required=False) - po = TestConstant(value=value) - assert po.value == value + po = P(value1=value, value2=value) + assert po.value1 == value + assert po.value2 == value @pytest.mark.parametrize("default", [None, "d"]) -def test_constant_parameter_raises(default): +def test_required_raises(default): """Test that you can make a parameter required and it will raise an error if not provided.""" - class TestConstant(param.Parameterized): - value = param.Parameter(default=default, required=True) + class P(param.Parameterized): + value1 = param.Parameter(default=default, required=True) + notrequired = param.Parameter(default=default, required=False) with pytest.raises( TypeError, - match=re.escape(r"__init__() missing 1 required positional argument: 'value'"), + match=re.escape(r"P.__init__() missing 1 required keyword-only argument: 'value1'"), ): - TestConstant() + P() + + class Q(param.Parameterized): + value1 = param.Parameter(default=default, required=True) + value2 = param.Parameter(default=default, required=True) + notrequired = param.Parameter(default=default, required=False) + + with pytest.raises( + TypeError, + match=re.escape(r"Q.__init__() missing 2 required keyword-only arguments: 'value1' and 'value2'"), + ): + Q() + + class R(param.Parameterized): + value1 = param.Parameter(default=default, required=True) + value2 = param.Parameter(default=default, required=True) + value3 = param.Parameter(default=default, required=True) + notrequired = param.Parameter(default=default, required=False) + + with pytest.raises( + TypeError, + match=re.escape(r"R.__init__() missing 3 required keyword-only arguments: 'value1', 'value2', and 'value3'"), + ): + R() + + +def test_required_inheritance(): + class A(param.Parameterized): + p = param.Parameter(default=1, required=True, doc='aaa') + + class B(A): + pass + + class C(B): + p = param.Parameter(required=False, doc='bbb') + + with pytest.raises( + TypeError, + match=re.escape(r"A.__init__() missing 1 required keyword-only argument: 'p'"), + ): + A() + + with pytest.raises( + TypeError, + match=re.escape(r"B.__init__() missing 1 required keyword-only argument: 'p'"), + ): + B() + + c = C() + + assert c.p == 1 + + C.param.p.required = True + + with pytest.raises( + TypeError, + match=re.escape(r"C.__init__() missing 1 required keyword-only argument: 'p'"), + ): + C() + + c = C(p=2) + + assert c.p == 2 @pytest.fixture From d10719f1ac1ade543c8272a903224554fd3bf391 Mon Sep 17 00:00:00 2001 From: maximlt Date: Mon, 1 May 2023 22:46:57 +0200 Subject: [PATCH 8/8] update docs --- examples/user_guide/Parameters.ipynb | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/user_guide/Parameters.ipynb b/examples/user_guide/Parameters.ipynb index 6bf8030f9..3f819bddf 100644 --- a/examples/user_guide/Parameters.ipynb +++ b/examples/user_guide/Parameters.ipynb @@ -35,6 +35,7 @@ "- **instantiate**: Whether to deepcopy the default value into a Parameterized instance when it is created. False by default for Parameter and most of its subtypes, but some Parameter types commonly used with mutable containers default to `instantiate=True` to avoid interaction between separate Parameterized instances, and users can control this when declaring the Parameter (see below). \n", "- **per_instance**: whether a separate Parameter instance will be created for every Parameterized instance created. Similar to `instantiate`, but applies to the Parameter object rather than to its value.\n", "- **precedence**: Optional numeric value controlling whether this parameter is visible in a listing and if so in what order.\n", + "- **required**: Whether a value must be provided on instantiation of a Parameterized object.\n", "\n", "Most of these settings (apart from **name**) are accepted as keyword arguments to the Parameter's constructor, with `default` also accepted as a positional argument:" ]