-
-
Notifications
You must be signed in to change notification settings - Fork 36
Implement changes to date/time functions #648
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
Conversation
This is per the 2024-02-14 teleconference.
- valid [Unicode Calendar Identifier](https://cldr-smoke.unicode.org/spec/main/ldml/tr35.html#UnicodeCalendarIdentifier) | ||
- `numberingSystem` (default is locale-specific) | ||
- valid [Unicode Number System Identifier](https://cldr-smoke.unicode.org/spec/main/ldml/tr35.html#UnicodeNumberSystemIdentifier) | ||
- `timeZone` (default is system default time zone or UTC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not having the timezone setting will make it difficult to test in server environments where the 'default' is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, even in the case where the implementation provides a message-context API for setting the time zone.
The argument is that you "ought" to use a Temporal type with real time zone support--which I don't disagree that one should do that. But there are lots of applications that can't or systems that do not provide anything but an incremental time value and need to set the time zone.
Not providing an option to set that seems problematic.
On the other hand, these specific options will likely enter Tech Preview as "RGI" and we'll get feedback from users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The system or locale time zone should never be implicitly used; I would suggest formatting with "GMT+?" if the time zone is not specified but the format requires a time zone. The correct way to pass in a time zone is to use a ZonedDateTime type or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shane has a good point. This is a bit like currencies with numbers; the input should include both, and if you don't have an input timezone, then the assumption is UTC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest formatting with "GMT+?" if the time zone is not specified but the format requires a time zone.
Yes, if the format requires a time zone and one is not available. Although that generally means a floating time value such as PlainDate
or LocalTime
. The system time zone will be wrong in a server context, but not all code runs in a server context. The correct way to handle this is to provide message-level contextual time zone (an implementation detail) including API access to same--not to produce shitty date/time display for users of non-temporal data types.
This is a bit like currencies with numbers; the input should include both, and if you don't have an input timezone, then the assumption is UTC.
The input should include both, absolutely. However, there are plenty of java.util.Date
and JS Date
objects in the world which are epoch times in UTC for which the display should be in some specific time zone. And Temporal isn't quite shipped in JS.
For these sorts of incremental time values, the ability to say:
Your package arrived: {$deliveryDate :datetime timeZone=$customerTZ}
... is sometimes useful.
I even have ample implementation experience of zoned values needing to be floated or re-zoned for display purposes. Doing this in Amazon's message formatter was easy to explain and worked effectively, although in our case we passed the time zone via the formatting context and it applied to the whole message, not to specific fields.
Note that there are no options for "floating" a time value for display (that is, turning an Instant
or Zoned*
into a Plain*
/Local*
etc.), which is a super-common thing to want to do at format time (or the inverse, removing an incorrect time zone but keeping the time fields constant). You could argue that developers should have to convert the value themselves, but its convenient to let time-handling experts do this (i.e. inside the MF implementation) rather than developers getting it wrong (explaining how to work with time values is hard work relatively speaking)
The system or locale time zone should never be implicitly used
I do not agree. It means that simple messages never display local time when using old-fashioned time value like java.util.Date
. I want to write the following and use System.currentTimeMillis()
or new Date()
if I feel like it:
This time is now {$now :time}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we're aligned but let me clarify. The system time zone can be used when localizing a timestamp (absolute time / instant). Ideally that happens outside of MF because MF shouldn't be in the business of date arithmetic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MF is definitely not in the anything arithmetic business 😄 but our functions can sometimes be.
What I have in mind here is that users be able (but not required) to pass a time zone when they need the underlying formatter to do one of the following:
- attach a floating time to the timeline (e.g.
ZonedDateTime.of(myLocalDateTime, zoneId)
) - change the time zone of a zoned time value
- set the time zone for displaying an unzoned instant (like
java.util.Date
)
As noted, this could be done in the formatting context, but an application might need to do any of these for a specific expression field:
The time {$now :time} is {$now :time timezone=$usersZone} in {$usersZone}.
Notice that in all three cases you want the value and the target time zone and the desired operation can be determined through reflection. Re-floating a time value isn't generally necessary in the formatter unless what you want to do is get rid of the time zone name in the display.
Ideally that happens outside of MF because MF shouldn't be in the business of date arithmetic.
As above: ideally yes. The option is for a non-ideal world.
One downside to the above: the option can take a value and the value can be an argument. But the value can't be null/empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the challenges with {$now :time timezone=$usersZone}
is defining, how it is supposed to behave based on the input type?
- Temporal.Absolute: probably combine the offset with the time zone and format as a ZonedDateTime
- Temporal.ZonedDateTime: a few things one could do in principle
- Keep the ZonedDateTime time zone
- Use the MF time zone and project the absolute timestamp into that time zone
- Use the MF time zone without changing the clock time of the ZonedDateTime
- Throw an exception
- Temporal.PlainDate: no time of day is available, so throw an exception?
- Temporal.PlainDateTime: a few things are possible
- Use the PlainDateTime at face value and attach the time zone, formatting as a ZonedDateTime
- Assume the PlainDateTime is in the system time zone and perform the conversion (yuck, don't do this)
- Temporal.PlainTime: similar to Temporal.PlainDateTime
Due to interoperability concerns, I do think MF2 should specify the data flow. I'm in full alignment that MF2 shouldn't specify output formats, but it should specify the flow of information so that two different implementations produce semantically equivalent output.
Field options describe which fields to include in the formatted output | ||
and what format to use for that field. | ||
Only those fields specified in the _annotation_ appear in the formatted output. | ||
|
||
The _field_ options are defined as follows: | ||
|
||
All functions have the following option: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeZoneName
should not be in :date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Consistent with `:number` doc
Co-authored-by: Shane F. Carr <sffc@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small stuff, inline.
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
Co-authored-by: Stanisław Małolepszy <sta@malolepszy.org>
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
Field options describe which fields to include in the formatted output | ||
and what format to use for that field. | ||
Only those fields specified in the _annotation_ appear in the formatted output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the features of semantic skeleta is preventing nonsensical skeletons like "hours and seconds" without minutes. Mapping from standard skeleta to semantic skeleta may fill in some of those missing fields. Even now, fields such as hour cycle and era may appear even if not in the skeleton. Therefore, it would be better to remove the second sentence giving any guarantees about which fields appear in the formatted output.
@@ -109,7 +117,7 @@ For more information, see [Working with Timezones](https://w3c.github.io/timezon | |||
> The [ABNF](/spec/message.abnf) and [syntax](/spec/syntax.md) of MFv2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above: there is no timezoneOffset
field. Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timezoneOffset
is a field in XMLSchema's various specified literal representations, not our spec.
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
This is per the 2024-02-14 teleconference.