feat: Create deephaven.time time-type aliases#5269
feat: Create deephaven.time time-type aliases#5269devinrsmith merged 7 commits intodeephaven:mainfrom
Conversation
devinrsmith
added
python
NoDocumentationNeeded
python-server-side
NoReleaseNotesNeeded
devinrsmith
requested review from
chipkent,
jmao-denver and
rcaudy
as code owners
There was a problem hiding this comment.
High level questions:
- Does PyCharm correctly type hint with this?
- Does VS Code correctly type hint with this?
- What do the Sphinx docs look like?
There was a problem hiding this comment.
I added simple pydocs to see what it looks like. I don't use PyCharm, so somebody else would need to check that.
| TimeZoneAlias = Union[TimeZone, str, datetime.tzinfo, datetime.datetime, pandas.Timestamp] | ||
| LocalDateAlias = Union[LocalDate, str, datetime.date, datetime.datetime, numpy.datetime64, pandas.Timestamp] | ||
| LocalTimeAlias = Union[LocalTime, int, str, datetime.time, datetime.datetime, numpy.datetime64, pandas.Timestamp] | ||
| InstantAlias = Union[Instant, int, str, datetime.datetime, numpy.datetime64, pandas.Timestamp] | ||
| ZonedDateTimeAlias = Union[ZonedDateTime, str, datetime.datetime, numpy.datetime64, pandas.Timestamp] | ||
| DurationAlias = Union[Duration, int, str, datetime.timedelta, numpy.timedelta64, pandas.Timedelta] | ||
| PeriodAlias = Union[Period, str, datetime.timedelta, numpy.timedelta64, pandas.Timedelta] |
There was a problem hiding this comment.
- These need pydocs for sphinx.
- I'm concerned that "Alias" might be confusing to users. Maybe "Union" or something else.
There was a problem hiding this comment.
To me, Alias is appropriate except for int, str. The same is for Type, Kind. I don't like Union that much either. I don't have a good suggestion and if I am forced to choose, I would pick one of 'Alias', 'Type', and 'Kind'.
devinrsmith
changed the title
| period = time.to_j_duration(period) | ||
|
|
||
| builder.period(period) | ||
| if period: |
There was a problem hiding this comment.
Confirming that an exception will be thrown if period is None. An early impl had a fixed value, which was not good. I think this fixed value was removed everywhere, but I'm not 100%.
There was a problem hiding this comment.
Good call; there was an error thrown, but it wasn't as obvious in the java layer. I've explicitly accounted for this now and added a test case.
4 participants
This adds
TimeZoneLike,LocalDateLike,InstantLike,ZonedDateTimeLike,DurationLike, andPeriodLikeas aliased union types for objects that can be coerced into said type. This makes it easier for public APIs to reference the proper time-types.S3Instructions,json.instant_val,TableReplayer,time_table, and thetime.to_j_APIs have been updated to reference the new types.