Skip to content

Conversation

mpharrigan
Copy link
Collaborator

fixes #5301

See #5301 for motivation

@mpharrigan mpharrigan requested review from dabacon and maffoo May 31, 2022 17:52
@mpharrigan mpharrigan requested review from a team, cduck and vtomole as code owners May 31, 2022 17:52
@CirqBot CirqBot added the size: S 10< lines changed <50 label May 31, 2022
Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

Minor comments, then LGTM

```

Protobuf APIs will return "aware" `datetime` objects. JSON de-serialization will
promote values to "aware" `datetime` objects upon deserialization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might also want to suggest that in cirq functions when we compare datetime instances (for equality or ordering) we should compare .timestamp() instead of comparing the datetime instances directly, since this will work for aware and naive datetimes just like serialization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wellllllllllllllll in #5301 (comment) (specifically the linked comment) the proposal was to compare the datetimes. This was in the context of dataclass-like member variables where the motivation was to mirror the behavior of the components. I suppose the tradeoffs could be different in functions where we just want it to work.

Propose adding:

Comparing (or testing equality) between "naive" and "aware" datetime objects throws
an exception.
If you are implementing a class that has datetime member variables, delegate equality
and comparison operators to the built-in datetime equality and comparison operators.
If you're writing a function that compares datetime objects, you can defensively promote
them to "aware" objects or use their .timestamp() properties for a comparison that will
never throw an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maffoo does this sound good?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 21, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jun 21, 2022
@CirqBot CirqBot merged commit 2f07700 into quantumlib:master Jun 21, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jun 21, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S 10< lines changed <50

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Datetime timezone design

4 participants