Fix time-related types and properties #1713
Conversation
| * Depending on the target language, code generators can keep the union or remove it and leniently parse | ||
| * strings to the target type. | ||
| */ | ||
| export type Stringified<T> = T | string |
There was a problem hiding this comment.
Why add this here and not in _spec_utils/behaviors.ts?
Also, if this is a behavior, you should add the @behavior tag.
There was a problem hiding this comment.
I initially considered it as a behavior, but it's not. Behaviors are interfaces whose TypeScript definition is essentially empty and are meant to capture things we cannot easily represent in TypeScript.
In this case, it's just a regular type, that code generators may or may not consider in a special way.
There was a problem hiding this comment.
Amazing work!
A couple of notes:
If Stringified is a behavior, you should also update:
elasticsearch-specification/compiler/src/model/utils.ts
Lines 55 to 61 in 8061875
The typescript generator must be updated as well.
|
Additional thoughts (cc @technige). We may consider a more generic way of defining values with units, as follows: export type UnitSeconds = long
export type UnitMillis = long
export type UnitNanos = long
export type UnitFloatMillis = double
/** A timestamp in time units since the Epoch */
export type EpochValue<TimeUnit> = TimeUnit
/** A timestamp in millis */
export type EpochMillis = EpochValue<UnitMillis>
/** A timestamp in seconds */
export type EpochSeconds = EpochValue<UnitSeconds>
export type TimeSpanValue<Unit> = Unit
export type TimeSpanSeconds = TimeSpanValue<UnitSeconds>
export type TimeSpanMillis = TimeSpanValue<UnitMillis>
export type TimeSpanNanos = TimeSpanValue<UnitNanos>
export type TimeSpanFloatMillis = TimeSpanValue<UnitFloatMillis>This allows capturing the essential nature of a type (e.g. it's an epoch-based timestamp) separately from its unit. Same would apply to byte sizes. For strongly typed languages this "essential nature" may map to a single type (or even a built-in type such as |
No need for that, since |
|
@swallez This is great and I appreciate the effort you've gone to here. Makes so much sense to standardise the types and their usage. I'm all for these changes. I much prefer the way |
|
@stevejgordon thanks for the feedback! I'll go ahead and add the I'm wondering if we should go even further and unify the number and text representations of date/time. The generic parameter would then not be a unit but a format (that also defines its unit). export type FmtSeconds = long
export type FmtMillis = long
export type FmtNanos = long
export type FmtFloatMillis = double
export type FmtIsoOrMillis = string | long
export type DateTime<Format> = Format
export type DateText = DateTime<FmtIsoOrMillis>
export type EpochMillis = DateTime<FmtMillis>
export type EpochSeconds = DateTime<FmtSeconds>
export type TimeSpan<Format> = Format
export type TimeSpanSeconds = TimeSpan<FmtSeconds>
export type TimeSpanMillis = TimeSpan<FmtMillis>
export type TimeSpanNanos = TimeSpan<FmtNanos>
export type TimeSpanFloatMillis = TimeSpan<FmtFloatMillis>Or this this going too far? Also, target is 8.3. Or 8.4 if we want to keep some room to implement this correctly, and will not be backported. |
|
General naming question: |
There was a problem hiding this comment.
All the work done in this PR is Herculean, excellent work @swallez! 💪 I've read every change top to bottom and left a bunch of small comments and a few questions too:
| @@ -48,7 +48,8 @@ export interface Request extends RequestBase { | |||
| * Specifies to advance to a particular time value. Results are generated | |||
| * and the model is updated for data from the specified time interval. | |||
| */ | |||
| advance_time?: DateString | |||
| // Also accepts `now` as a value, epoch seconds (< 10 digits) and epoch milliseconds | |||
| advance_time?: DateTime | |||
There was a problem hiding this comment.
Should we capture the DateTime | EpochMillis aspect of this type? The EpochSeconds seems like it'd be hacky to annotate.
There was a problem hiding this comment.
I'd rather not expose the support for epoch seconds (even if it's mentioned here as a comment, i.e. not output in schema.json) as it's hacky and brittle.
Also "now" fits in the string variant of DateTime since the accepted format includes but is not limited to ISO 8601, depending on the type/property.
"Period" implies repetition, while this type is for duration measurements, duration settings (e.g. timeouts), intervals and sometimes indeed periods. So "Period" is too specific IMHO. Also ISO 8601 says "The use of the character P is based on the historical use of the term “period” for duration" 😅 Also we have two TimeSpan types:
If we look at some of our languages:
We have a rather large variety of names, and I'm happy to use something else than TimeSpan (except Period 😉). Thoughts? |
|
Sounds like |
Ok for |
|
@sethmlarson I took all of your judicious comments into account (thanks for the thorough review) and found even more wrongly typed fields ;-) I also added the "units as generic parameters" approach we discussed and we now have: export type UnitSeconds = long
export type UnitMillis = long
... other units
export type EpochTime<Unit> = Unit
export type DateTime = string | EpochTime<UnitMillis>
export type DurationValue<Unit> = Unit
export type Duration = string | -1 | 0Are we ok with this naming, in particular |
|
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
| @@ -54,24 +53,10 @@ export interface Request extends RequestBase { | |||
| * @server_default false | |||
| */ | |||
| ignore_unavailable?: boolean | |||
There was a problem hiding this comment.
Nice work finding these extra parameters 👍
Supersedes #1673
This PR does a big overhaul of time handling in the API spec: it removes redundant time-related types that existed, and refines the available types to capture the precise representation sent or expected by Elasticsearch.
This is a breaking change, in two main areas:
Full validation fixes a number of errors and does not add new ones.