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

Conversation

taldcroft
Copy link
Member

Description

This changes the TypedDescriptor behavior so that the default is only coerced to the type at the time of instance creation. This is important for the CxoTimeDescriptor in sot/cxotime#43, but in general this lazy evaluation of the default is a good thing.

Interface impacts

None

Testing

Unit tests

  • Mac
(razl) ➜  ska_helpers git:(typed-descriptor-lazy-default) git rev-parse HEAD                                                        
d7f2e189eb2fb46a30d3d801f125da1eacc3d0c8
(razl) ➜  ska_helpers git:(typed-descriptor-lazy-default) pytest
=========================================================================== test session starts ============================================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 80 items                                                                                                                                                         

ska_helpers/retry/tests/test_retry.py ..........                                                                                                                     [ 12%]
ska_helpers/tests/test_chandra_models.py ..................                                                                                                          [ 35%]
ska_helpers/tests/test_git_helpers.py s                                                                                                                              [ 36%]
ska_helpers/tests/test_intervals.py .................                                                                                                                [ 57%]
ska_helpers/tests/test_paths.py ......                                                                                                                               [ 65%]
ska_helpers/tests/test_run_info.py ..                                                                                                                                [ 67%]
ska_helpers/tests/test_utils.py ..........................                                                                                                           [100%]

====================================================================== 79 passed, 1 skipped in 7.48s =======================================================================

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

Tested also in sot/cxotime#43. In particular the distinction between evaluation at instance creation vs. class creation is demonstrated there.


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.

@taldcroft taldcroft merged commit f21bfd7 into master Jan 9, 2024
2 checks passed
@taldcroft taldcroft deleted the typed-descriptor-lazy-default branch January 9, 2024 20:09
@javierggt javierggt mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants