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

Specify OffsetDateTime, LocalDateTime, LocalDate, and LocalTime types. #414

Merged
merged 3 commits into from
Jan 3, 2017

Conversation

mojombo
Copy link
Member

@mojombo mojombo commented May 14, 2016

This is a possible solution for the concerns raised in #362. I'd like to have a conversation around a concrete spec. I have to say that this turns out not to feel too complex for me, and solves almost every issue that has been brought up.

The bit about how to handle conversion from LocalDateTime when the interpreted instant would either not exist, or exist twice, leads to ambiguity, but I phrased it as such to simplify parser construction. Most languages will have some built-in approach to resolving the instant, but I assume they will differ slightly, and over-specifying the TOML spec could make the parsing a nightmare. I figure that if this kind of ambiguity is unacceptable to a user, they can simply use an OffsetDateTime. I'm curious to hear thoughts on the matter.

@RobertBColton
Copy link

RobertBColton commented May 14, 2016

I am not sure if it's ambiguous, but I believe there is an argument to made also that just time itself should be a separate type from datetime and date as I explained in #412.

@Hrxn
Copy link

Hrxn commented May 14, 2016

I agree...

For the sake of consistency, include all four variants.

The two basics:

  • LocalDate
  • LocalTime

(Think just Date and Time, if someone thinks that Local may be misleading.)

Two Date and Time types:

  • LocalDateTime
  • OffsetDateTime

(That is, one with timezone and one without)

@ChristianSi
Copy link
Contributor

ChristianSi commented May 14, 2016

@mojombo: From the description of "Local Date", I would delete ", interpreted with the local timezone" and the following sentence. Dates are inherently timezone-less, as I've argued before. A timezone would be needed when converting the beginning or end of a Date into a DateTime, but they serve no useful role when just passing the whole Date around.

Therefore I'd also consider renaming "Local Date" to just "Date".

Otherwise this looks very good! 👍

@RobertBColton @Hrxn: I don't see compelling real-world use cases where dateless Times make sense, therefore I'd leave them out. The only use case seems for crontab-like stuff, where an event takes place at certain multiples of 24 hours. But for that kind of thing, you'd also need stuff such as "7 mins after every hour" or "every 15 mins" which wouldn't fit into a Time object either. So applications that need that kind of stuff will need to define their own type system anyway and are probably better off using TOML strings for that purpose.

@RobertBColton
Copy link

@ChristianSi I can think of a few other use cases where only time would be needed. One of them would be a preferences panel for some application that allows you to configure periodic backups. In my particular case I described in #412 how we are generating a UI from the toml file and why having the separate type would be useful with a GUI framework. In our case we are going to switch to schema's which will solve our problem, but I still believe there is an argument for having time as a separate type.

@jodastephen
Copy link

The local date-time and local date sections used a different concept of LocalDateTime and LocalDate to that defined in Java or ISO-8601. The meaning of "local date-time" is that the time-zone and offset is unspecified, not that it is in the local time-zone. A birth day is 1970-01-01, not 1970-01-01:00:00+02:00 - ie. you celebrate your birthday on the date wherever you are in the world, not based on the time-zone of your birth. As written therefore the PR is simply wrong IMO. However the changes necessary are not complex, see below.

Local time is a relatively common concept, occurring in shop opening hours, daily jobs, GUIs etc. It also occurs when there is a need to send date and time separately, something which I have used more than once. Not including local time would be a mistake IMO.

For instants, I think you need to specify how excess precision is handled:

"If the message contains greater precision than the implementation can support, the additional precision must be truncated, not rounded."


My proposed spec (plus the fractional precision part above):

Offset Date-Time

"A parser should return a distinct data type representing a date-time with offset, such as OffsetDateTime in Java. If the parser is unable to do this, it should convert the value to an instant representing the same moment on the time-line, a process that effectively loses the offset."

Local Date-Time

"If you omit the offset from an RFC 3339 formatted date-time, the date-time implies no offset or timezone."

"A parser should return a distinct data type representing a date-time without offset or timezone, such as LocalDateTime in Java. If the parser is unable to do this, it should convert the value to an instant using the local timezone. If the date-time does not exist, or is ambiguous, in the local timezone (e.g. because of daylight savings time boundaries), then it should be adjusted to a valid instant. At a parser's discretion, the local timezone may be configurable."

Local Date

"If you include only the date portion of an RFC 3339 formatted date-time, it will represent that entire day, with no offset of timezone implied."

"A parser should return a distinct data type representing a date without offset or timezone, such as LocalDate in Java. If the parser is unable to do this, it should convert the value to an instant using the local timezone. The conversion must select the first valid instant on the date. At a parser's discretion, the local timezone may be configurable."

Local Time

"If you include only the time portion of an RFC 3339 formatted date-time, it will represent time without reference to a date, offset or timezone."

T07:32:00

"A parser should return a distinct data type representing a time without offset or timezone, such as LocalTime in Java. If the parser is unable to do this, it should convert the value to an instant using the local timezone on the 1st January 1970. The conversion must select the first valid instant on the date. At a parser's discretion, the local timezone may be configurable."

@ChristianSi
Copy link
Contributor

Has this PR gone dormant or is it still alive?

I must admit that I find it pretty sad that the TOML standard seems to be slowly dying due to a lack of active maintainership :(

@Hrxn
Copy link

Hrxn commented Jun 4, 2016

Everyone and everything is slowly dying..

@mojombo
Copy link
Member Author

mojombo commented Jun 6, 2016

Ok, all, take a look at the changes I just pushed.

@ChristianSi @jodastephen You're right, the "Local" data types should be timezone-less, and I've updated the proposal to reflect that.

@ChristianSi I like having the "Local" on the types to differentiate them from the "Offset" type. It makes it explicit that these types do not contain enough information to be resolved into instants without further information (such as a local timezone). And yes, still alive!

@jodastephen I simply removed the wording about conversion using a local timezone, as each implementation and/or application can decide how to best do that for the given need. I added the bit about truncation of excess precision.

I also added a spec for a Time type, to see what that would look like. It does seem like it should be included, if Date is to be included, for completeness and obviousness.

@mojombo mojombo changed the title Specify OffsetDateTime, LocalDateTime, and LocalDate types. Specify OffsetDateTime, LocalDateTime, LocalDate, and LocalTime types. Jun 6, 2016
@ChristianSi
Copy link
Contributor

👍

@jodastephen
Copy link

My main comment is that the ISO-8601 standard specifies local time as "T11:30", not "11:30". I don't mind if you choose to go with "11:30", but parsers should be encouraged to accept and ignore a leading "T" if you do.

With "Conversion to an instant is implementation specific.", I would tweak to "Conversion to an instant, if required, is implementation specific." (If a language has a complete date and time library then there is no need to convert, and the "if required" provides an indication of that in the spec since you weren't convinced by my previous suggestions)

@ChristianSi
Copy link
Contributor

@jodastephen I'm against allowing 'T' at the start of LocalTime object's as it's surprising and violates usual practice. For example, Python's datetime.time objects stringify themselves in the form "11:33:45" and the toString method of Java's java.time.LocalTime is documented as follows: "The output will be one of the following ISO-8601 formats: HH:mm, HH:mm:ss, HH:mm:ss.SSS, HH:mm:ss.SSSSSS, HH:mm:ss.SSSSSSSSS."

I don't know about the ISO-8601 standard as it's nonfree and most people here will probably never see it, but I've heard that it's very complex and allows a lot of variety -- RFC 3339 was created exactly to reduce that complexity. And one of the rules from that RFC is:

 partial-time = time-hour ":" time-minute ":" time-second [time-secfrac]

That's the one to stick with. There is no rule that would allow a partial-time after 'T'.

@BurntSushi
Copy link
Member

I feel like having a time-only type is pretty weird, and the most compelling argument I've seen for its existence is "because consistency." One practical problem I'd be concerned with in particular is language support for "time only" data structures. How many languages have a time-only data structure that isn't tied to any particular date? It just seems like a very niche data type that we could do without.

@Hrxn
Copy link

Hrxn commented Jun 9, 2016

Java?

@RobertBColton
Copy link

@OptiverTimAll
Copy link

Also Python and Rust.

That said, since a time literal wouldn't be valid syntax for any other kind of literal, it could be compatibly added after 1.0, right?

@jodastephen
Copy link

I'd rather see these changes go in as is than argue about the format of the time string. I rechecked, and ISO-8601 does allow time without a prefix, so we are fine there. As the last three comments indicate, better date/time objects are making their way out to languages, so the data formats should represent them too.

@erichiller
Copy link

erichiller commented Jun 24, 2016

Suggestion: How about a single multi-purpose variable-length field of time, length determines precision.

  1. YYYY-MM-DD
  2. YYYY-MM-DD HH:NN[:SS]
  3. YYYY-MM-DD HH:NN[:SS] +/-O:FF

where NN is minutes and O is offset hours, FF is offset minutes (for the outlier cases)

In my opinion it is simple, readable, and obvious to the user what is going on. Also, it would be relatively easy to parse. Just my two cents. And I too would really like to see this standard locked down - love toml and too many times have I seen waiting for version 1.0

@dermariusz
Copy link

My idea would be to enclose time either in angle brackets eg. <YYYY-MM-DD> or low dashes _YYYY-MM-DD_. That provides simpler detection and validation for parsers. In that way parsers wouldn't go too complex.

@TheElectronWill
Copy link
Contributor

To my mind, surrounding time with brackets or low dashes makes it more complicated for the user to write a TOML file, and isn't necessary. Is isn't complicated to detect time or date (because remember, bare strings aren't allowed as values).

atweiden pushed a commit to atweiden/config-toml that referenced this pull request Aug 23, 2016
- currently forward looking, but at least this much of the spec seems
  destined to get merged upstream
  - toml-lang/toml#414
- currently missing a Local Time implementation
  - how does Local Time map to a Perl6 type?
atweiden pushed a commit to atweiden/txn-parser that referenced this pull request Aug 23, 2016
@skystrife
Copy link
Contributor

@mojombo @BurntSushi I think most of the comments after the last revision by Tom have been resolved or are about what color to paint the bike shed. We also appear to have the blessing of @jodastephen. Any chance of this getting merged soon?

@skystrife
Copy link
Contributor

It's now been four months since the last change to this PR. Can this be merged? Is there anything blocking this?

@skystrife
Copy link
Contributor

@mojombo @BurntSushi Friendly reminder that this is still unresolved.

@mojombo mojombo merged commit 2e18551 into master Jan 3, 2017
@mojombo
Copy link
Member Author

mojombo commented Jan 3, 2017

“Always in motion is the future.” —Yoda

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