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

Allow less specific datetimes. #297

Merged
merged 1 commit into from
Oct 16, 2015
Merged

Allow less specific datetimes. #297

merged 1 commit into from
Oct 16, 2015

Conversation

mojombo
Copy link
Member

@mojombo mojombo commented Feb 6, 2015

This allows three kinds of datetimes:

# Full RFC 3339
1979-05-27T07:32:00Z
1979-05-27T00:32:00-07:00
1979-05-27T00:32:00.999999-07:00

# No local offset
1979-05-27T07:32:00
1979-05-27T00:32:00.999999

# No local offset or time
1979-05-27

See #263 for why this is being proposed.

@cies
Copy link
Contributor

cies commented Feb 7, 2015

The precision of fractional seconds is implementation specific and no minimum is
expected. If the implementation's precision is exceeded, the datetime should
still be accepted without error (though a warning should be issued, if
possible).

So the following is valid?

1979-05-27T00:32:00.

How about dates before epoch? Or before the year zero?

This also introduces the concept of a parse warning... Before it would be valid or invalid, after this it may become valid+warning. Everyone app that uses the TOML format should now decide if/how it wants to deal with warnings (tell the user? log it somewhere?).

Please consider that date/time/timezones/timestamps is a can of worms that we do not want to open upon a minimal config format.

Quick proof: http://en.wikipedia.org/wiki/Leap_second

More proof in this blog post and sequel, most of the falsehoods mentioned are not worth considering in the context of a serialization format, some of them certainly are.

@ChristianSi
Copy link
Contributor

Looks good to me 👍

@cies As for your questions, it's best to consult RFC 3339. For examples, leap seconds are addressed by the RFC (seconds in the grammar range from 0 to 60 because of that).

Warnings are optional ("if possible") so I don't see that as a big issue.

@mojombo
Copy link
Member Author

mojombo commented Feb 11, 2015

@spf13 Can you weigh in on this proposal? Would it solve your needs?
@parkr I'd love to hear from you as well, from a Jekyll perspective.

@spf13
Copy link

spf13 commented Feb 11, 2015

I think this would work quite well. One of the odd behaviors today is the very strict format. I think this would be a real boost to adoption. I can see the no offset being commonly used. It would definitely work well for Hugo's current use case.

@mojombo
Copy link
Member Author

mojombo commented Oct 16, 2015

I'm merging this right now, as I think it will make TOML more pragmatically useful, but I'd enjoy a discussion about whether the bit about warnings should be changed to just be an error.

mojombo added a commit that referenced this pull request Oct 16, 2015
@mojombo mojombo merged commit 7a6e708 into master Oct 16, 2015
@spf13
Copy link

spf13 commented Oct 16, 2015

Thanks @mojombo . I'm happy to pass along any feedback I hear about it.

@ChristianSi
Copy link
Contributor

Looks good 👍

I'm in favor of leaving the warning as is, it feels like an awful waste lo let a parser die just because it has to crop 12.345678 secs to 12.345 (or 12.346) secs.

Warnings might also be appropriate in other situations, e.g. when a key occurs twice in a table. The spec says "Doing so is invalid" -- leaving it up to the implementor whether to throw a parsing error or emit a warning, I guess.

@ChristianSi
Copy link
Contributor

On second thought, maybe change

The precision of fractional seconds is implementation specific and no minimum is expected.

to

The precision of fractional seconds is implementation specific, but at least millisecond precision is expected.

?

Rounding 12.345678 secs to 12 secs would feel a bit rogue to me.

If millisecond precision is the common minimum, then I think it would be OK to make the warning optional ("may" instead of "should").

atweiden pushed a commit to raku-community-modules/Config-TOML that referenced this pull request Oct 17, 2015
- integrated less specific datetime TOML grammar from
  toml-lang/toml#297
- configure date local offset for TOML date values omitting the offset
  using the :date-local-offset parameter in from-toml, or by instantiating
  Actions with :date_local_offset parameter defined
  - the default is to use the host machine's local offset for TOML date
    values omitting the offset
@HuwCampbell
Copy link

It's quite dangerous to treat raw days as is suggested. For example in Brazil on the 18th of October, there is no 00:00:00 time at all, as with daylight savings it skips straight to 1 am (this gem has caused, for example, popular issue trackers to not allow issues to be defined on this day).

One can have a raw days implementation, but it is very different to a day's midnight in a timezone (it's a completely different datatype). ISO 8601 also allows this (but is not what you're implementing anymore).

This change makes TOML not precise, which is bad:tm:.

@HuwCampbell
Copy link

I think warnings opens up a bit of a can of worms too; could one, for instance, turn on -Wall and fail?

@mojombo
Copy link
Member Author

mojombo commented Oct 20, 2015

@HuwCampbell That's fair, but I still think this change is needed. How about suggesting noon as the time-point for raw days? Every day, regardless of daylight savings or other leap-changes has a noon.

@mojombo
Copy link
Member Author

mojombo commented Oct 20, 2015

The precision of fractional seconds is implementation specific, but at least millisecond precision is expected.

@ChristianSi I like this idea, it's consistent with the float spec (some minimum precision is expected), and just leave out the warning suggestion. Any implementation is free to warn on any number of cases that may cause issues for it. I'll work up a PR.

@mojombo
Copy link
Member Author

mojombo commented Oct 20, 2015

@ChristianSi See #360 for the change regarding milliseconds.

@mojombo
Copy link
Member Author

mojombo commented Oct 20, 2015

@HuwCampbell See #361 for the noon-time change.

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.

5 participants