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

[DS][26/n] Update internal representation of InLatestTimeWindow #21615

Merged
merged 1 commit into from
May 14, 2024

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

Updates the internal representation to better match the timedelta class. The painful thing here is that you can't really set a constructor when using pydantic models.

Not a huge fan of representing "no timedelta at all" as "all the things are 0"

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 3, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart force-pushed the 05-02-Narrow_extra_state_type branch from bf37d9b to c3c890f Compare May 3, 2024 16:58
@OwenKephart OwenKephart force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from 5256f73 to d63a0f5 Compare May 3, 2024 16:58
@OwenKephart OwenKephart requested a review from schrockn May 3, 2024 17:41
Comment on lines 53 to 55
lookback_days: int = 0
lookback_seconds: int = 0
lookback_microseconds: int = 0

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth nothing as comment that

>>> timedelta().days
0
>>> timedelta().seconds
0
>>> timedelta().microseconds
0

I went to the REPL and checked this to ensure it wasn't None.

Copy link
Member

Choose a reason for hiding this comment

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

And actually I think you should make these optional to distinguish from the zero case.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Let's make the serialized representation nullable to distinguihs from the default/empty timedelta.

I also recommend making it a tuple so there is only one Noneable property.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Change serialization format

@OwenKephart OwenKephart force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from d63a0f5 to c1120be Compare May 3, 2024 18:52
@OwenKephart OwenKephart force-pushed the 05-02-Narrow_extra_state_type branch from c3c890f to 3ccd4be Compare May 3, 2024 21:11
@OwenKephart OwenKephart force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from c1120be to 102f170 Compare May 3, 2024 21:11
@OwenKephart OwenKephart force-pushed the 05-02-Narrow_extra_state_type branch from 3ccd4be to 7b5f20e Compare May 3, 2024 22:29
Base automatically changed from 05-02-Narrow_extra_state_type to master May 3, 2024 22:31
@OwenKephart OwenKephart force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from 102f170 to a226325 Compare May 3, 2024 22:51
Copy link
Contributor Author

OwenKephart commented May 14, 2024

Merge activity

  • May 14, 10:38 AM EDT: @OwenKephart started a stack merge that includes this pull request via Graphite.
  • May 14, 10:40 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 14, 10:43 AM EDT: @OwenKephart merged this pull request with Graphite.

@OwenKephart OwenKephart force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from f9c81a0 to e678b9c Compare May 14, 2024 14:39
@OwenKephart OwenKephart merged commit 0a8d311 into master May 14, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 05-02-Update_values_within_InLatestTimeWindow branch May 14, 2024 14:43
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
…ter-io#21615)

## Summary & Motivation

Updates the internal representation to better match the timedelta class. The painful thing here is that you can't really set a constructor when using pydantic models.

Not a huge fan of representing "no timedelta at all" as "all the things are 0"

## How I Tested These Changes
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.

None yet

2 participants