Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coerce TypedDescriptor default at instance creation #54

Merged
merged 1 commit into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion ska_helpers/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,12 @@ class MyClass:
assert obj.val_int == 10

with pytest.raises(
ValueError, match="cannot set required attribute 'val_int' to None"
ValueError, match="attribute 'val_int' is required and cannot be set to None"
):
obj.val_int = None

with pytest.raises(
ValueError, match="attribute 'val_int' is required and cannot be set to None"
):
MyClass()

Expand All @@ -198,6 +203,9 @@ def test_int_descriptor_has_default(cls_descriptor):
class MyClass:
val_int: int = cls_descriptor(default=10.5)

# Accessing the class attribute returns original default value (used by dataclass).
assert MyClass.val_int == 10.5

obj = MyClass()
# Default of 10.5 is cast to int
assert obj.val_int == 10
Expand Down
50 changes: 36 additions & 14 deletions ska_helpers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,26 +375,36 @@ def convert_to_int_float_str(val: str) -> int | float | str:


class TypedDescriptor:
"""Class to create a descriptor for an attribute that is cast to a type.
"""Class to create a descriptor for a dataclass attribute that is cast to a type.

This is a base class for creating a descriptor that can be used to define an
attribute on a class that is cast to a specific type. The type is specified by
attribute on a dataclass that is cast to a specific type. The type is specified by
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a similar comment to the one I left in the cxotime PR. I'm not sure you have to restrict yourself to dataclasses. I think it might happen that you want to use the descriptor in a class and still not use a dataclass, in which case the constructor needs the proper initialization, and perhaps that should be documented.

This is the example I gave there (is this a reasonable example?):

class MyClass_1:
    time: CxoTime = CxoTimeDescriptor(default=CxoTime.NOW)
    def __init__(self, **kwargs):
        self.time = MyClass.time

class MyClass_2:
    time: CxoTime | None = CxoTimeDescriptor(default=None)
    def __init__(self, **kwargs):
        self.time = MyClass.time

Copy link
Member Author

@taldcroft taldcroft Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javierggt - you are right this can work in a normal class. The issue there is that it is easy to make a mistake. Your example does not work if you pass in a time=value to the class constructor. It should be:

In [10]: class MyClass_1:
    ...:     time: CxoTime = CxoTimeDescriptor(default=CxoTime.NOW)
    ...: 
    ...:     def __init__(self, time=None, **kwargs):
    ...:         self.time = time or MyClass_1.time

The beauty of dataclasses is to reduce the boilerplate in the __init__. I'm moving to more concrete classes where all the attributes are well-defined from the start, so dataclasses are making a lot of sense.

For the documentation in this case I think less is more. Experts who understand all the machinery don't need the documentation examples, and the rest should only use this in a dataclass as the intended and tested use-case.

setting the ``cls`` class attribute on the descriptor class.

Most commonly ``cls`` is a class like ``CxoTime`` or ``Quat``, but it could also
be a built-in like ``int`` or ``float`` or any callable function.
Most commonly ``cls`` is a class like ``CxoTime`` or ``Quat``, but it could also be
a built-in like ``int`` or ``float`` or any callable function.

This descriptor can be used either as a base class with the ``cls`` class attribute
set accordingly, or as a descriptor with the ``cls`` keyword argument set.

.. warning:: This descriptor class is recommended for use within a
`dataclass <https://docs.python.org/3/library/dataclasses.html>`_. In a normal
class the default value **must** be set to the correct type since it will not be
coerced to the correct type automatically.

The default value cannot be ``list``, ``dict``, or ``set`` since these are mutable
and are disallowed by the dataclass machinery. In most cases a ``list`` can be
replaced by a ``tuple`` and a ``dict`` can be replaced by an ``OrderedDict``.

Parameters
----------
default : optional
Default value for the attribute. If not specified, the default for the
attribute is ``None``.
Default value for the attribute. If specified and not ``None``, it will be
coerced to the correct type via ``cls(default)``. If not specified, the default
for the attribute is ``None``.
required : bool, optional
If ``True``, the attribute is required to be set explicitly when the object
is created. If ``False`` the default value is used if the attribute is not set.
If ``True``, the attribute is required to be set explicitly when the object is
created. If ``False`` the default value is used if the attribute is not set.

Examples
--------
Expand Down Expand Up @@ -439,24 +449,36 @@ def __init__(self, *, default=None, required=False, cls=None):
self.cls = cls
if required and default is not None:
raise ValueError("cannot set both 'required' and 'default' arguments")
self.default = default if default is None else self.cls(default)
self.required = required

# Default is set here at the time of class creation, not at the time of
# instantiation. Coercing the default to the correct type is deferred until the
# instance is created. This happens because at instance creation (if the
# attribute value was not specified) the dataclass machinery evaluates
# ``Class.attr`` (e.g. ``QuatDescriptor.quat``) which triggers the ``__get__``
# method of the descriptor with ``obj=None``. That returns the default which is
# then passed to the ``__set__`` method which does type coercion. See
# https://docs.python.org/3/library/dataclasses.html#descriptor-typed-fields and
# the bit about "To determine whether a field contains a default value".
self.default = default

def __set_name__(self, owner, name):
self.name = "_" + name

def __get__(self, obj, objtype=None):
if obj is None:
# See long comment above about why this is returning self.default.
return self.default

if self.required:
return getattr(obj, self.name)
else:
return getattr(obj, self.name, self.default)
return getattr(obj, self.name)

def __set__(self, obj, value):
if self.required and value is None:
raise ValueError(f"cannot set required attribute {self.name[1:]!r} to None")
raise ValueError(
f"attribute {self.name[1:]!r} is required and cannot be set to None"
)
# None is the default value for the attribute if it is not set explicitly.
# In this case it is not coerced to the descriptor type.
if value is not None:
value = self.cls(value)
setattr(obj, self.name, value)
Loading