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

Add CxoTimeDescr descriptor and CxoTime.NOW sentinel #43

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Dec 17, 2023

Description

This adds a descriptor that allows for dataclass attributes that are cast as CxoTime. This is most obvious looking at the test or docs.

This also adds a new class attribute CxoTime.NOW which is the new preferred way to specify the current time. Functions that used to be like

def func(stop=None):
    stop = CxoTime(stop)
    ...

should now be:

def func(stop=CxoTime.NOW):
    stop = CxoTime(stop)
    ...

While in the package I gave the cxotime.py module an __all__ attribute.

Requires

Interface impacts

None

Testing

Unit tests

  • Mac (new tests)
(masters) ➜  cxotime git:(cxotime-descr) git rev-parse HEAD                                                             
f0f3d24d17f318c54b483e644ead80142b7439e1
(masters) ➜  cxotime git:(cxotime-descr) 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 235 items                                                                                                            

cxotime/tests/test_cxotime.py .......................................................................................... [ 38%]
........................................................................................................................ [ 89%]
.........................                                                                                                [100%]

===================================================== 235 passed in 2.15s ======================================================

Independent check of unit tests by Jean

  • Linux
  • Mac (output below)
(ska3) flame:cxotime jean$ git rev-parse HEAD
f0f3d24d17f318c54b483e644ead80142b7439e1
(ska3) flame:cxotime jean$ python -c "import ska_helpers; print(ska_helpers.__version__);"
0.13.1.dev8+gd7f2e18
(ska3) flame:cxotime jean$ pytest
=================================================================================== test session starts ===================================================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/jean/git/cxotime
plugins: timeout-2.1.0, anyio-3.6.2
collected 235 items                                                                                                                                                                       

cxotime/tests/test_cxotime.py ..................................................................................................................................................... [ 63%]
......................................................................................                                                                                              [100%]

=================================================================================== 235 passed in 2.50s ===================================================================================
(ska3) flame:cxotime jean$ 

Functional tests

No functional testing.

cxotime/cxotime.py Outdated Show resolved Hide resolved
@jeanconn jeanconn self-requested a review January 4, 2024 18:39
Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

It is unclear to me if this was going to stay as-is or have small updates based on the Descr conversations yesterday.

@taldcroft
Copy link
Member Author

@jeanconn - this is getting updated, but things are a little in flux now so stand by. I'm trying to introduce a NOW sentinel value but having some trouble with the descriptor stuff.

@taldcroft
Copy link
Member Author

@jeanconn - this is now down to a simple subclass of TypedDescriptor and the main thing is to review the docs and tests. So it is ready for review again.

@javierggt - for this iteration there is no convenient way to set a default NOW. I played with a NOW sentinel but this got into a bit of a mess 1 so I backed that out. Basically if you want NOW then it would be like:

@dataclass
class MyClass:
    stop: CxoTime | None = CxoTimeDescriptor()

obj = MyClass(stop=CxoTime.now())

Footnotes

  1. Right now the TypedDescriptor coerces the default value to the type when the dataclass is created, NOT when the dataclass instance is created. I tried changing that in a way that seemed reasonable to me but this gave a very long an inscrutable error from within the dataclass decorator magic, so I gave up for now.

@taldcroft taldcroft changed the title Add CxoTimeDescr descriptor and minor tidy Add CxoTimeDescr descriptor and CxoTime.NOW sentinel Jan 6, 2024
@taldcroft taldcroft requested a review from jeanconn January 6, 2024 12:20
Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

The new NOW looks to be working but I've got failed tests with sot/ska_helpers#54 in PYTHONPATH .

================================================================================= short test summary info =================================================================================
FAILED cxotime/tests/test_cxotime.py::test_cxotime_descriptor_is_required - AssertionError: Regex pattern did not match.
FAILED cxotime/tests/test_cxotime.py::test_cxotime_descriptor_with_NOW - AssertionError: assert 0.0 == 0.5
============================================================================== 2 failed, 233 passed in 1.78s ==============================================================================

@taldcroft
Copy link
Member Author

taldcroft commented Jan 8, 2024

@jeanconn - I'm wondering if you made a mistake somehow? This is all looking good for me.

(masters) ➜  cxotime git:(cxotime-descr) git rev-parse HEAD                                                             
f0f3d24d17f318c54b483e644ead80142b7439e1
(masters) ➜  cxotime git:(cxotime-descr) 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 235 items                                                                                                     

cxotime/tests/test_cxotime.py ................................................................................... [ 35%]
................................................................................................................. [ 83%]
.......................................                                                                           [100%]

================================================== 235 passed in 2.22s ==================================================
(masters) ➜  cxotime git:(cxotime-descr) python -c 'import ska_helpers; print(ska_helpers.__version__)'
0.13.1.dev8+gd7f2e18
(masters) ➜  cxotime git:(cxotime-descr) ska3
(ska3) ➜  cxotime git:(cxotime-descr) env PYTHONPATH=/Users/aldcroft/git/ska_helpers pytest --pdb
===================================================== 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 235 items                                                                                                           

cxotime/tests/test_cxotime.py ......................................................................................... [ 37%]
....................................................................................................................... [ 88%]
...........................                                                                                             [100%]

===================================================== 235 passed in 2.37s =====================================================

@jeanconn
Copy link
Contributor

jeanconn commented Jan 8, 2024

I did make a mistake and had a typo in PYTHONPATH so I wasn't getting the version of ska_helpers I thought. Thanks!

Parameters
----------
default : CxoTimeLike, optional
Default value for the attribute which is provide to the ``CxoTime`` constructor.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: provided

>>> dt = obj2.stop - obj1.stop
>>> round(dt.sec, 2)
2.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also say that it can be used in a class (not a dataclass) as long as the constructor initializes the attribute.

I checked that the following two worked as I expected.

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

Is there something else that one would need to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

@taldcroft taldcroft merged commit 2bbbaf2 into master Jan 9, 2024
2 checks passed
@taldcroft taldcroft deleted the cxotime-descr 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.

3 participants