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

Clarify sub-millisecond precision is allowed #805

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aduh95
Copy link

@aduh95 aduh95 commented Jan 23, 2021

"Millisecond precision is required" could be interpreted as a requirement for input to be valid TOML. This commit adds examples of valid TOML time values that uses tens of second to remove the ambiguity.

Refs: toml-rs/toml-rs#416
Co-Authored-By: Dave Ostroske [email protected]

"Millisecond precision is required" could be interpreted as a
requirement for input to be valid TOML. This commit adds examples of
valid TOML time values that uses tens of second to remove the ambiguity.
@ChristianSi
Copy link
Contributor

I don't think this is necessary, but it certainly cannot hurt.

Copy link

@bl-ue bl-ue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻

Copy link
Contributor

@eksortso eksortso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ChristianSi ChristianSi mentioned this pull request Oct 28, 2022
@eksortso
Copy link
Contributor

However, upon reflection, I think a little more explanation in the text is necessary. We currently state that "millisecond precision is required," but it's unclear that we are referring to the parsers' level of support for precision, not the users' precision, which should be arbitrary. Perhaps we ought to change the precision text to read as follows:

Implementations should support at least millisecond precision. Additional digits of precision may be specified, but if they exceed the supported precision, then the extra digits must be truncated, not rounded.

Then, if it wouldn't be too much detail, we could indicate what the resulting values would look like by referring back to the examples, to make this crystal clear.

For instance, a specified value of 00:32:00.999999 would be truncated to 00:32:00.999 when parsed with millisecond precision.

Thoughts?

@ChristianSi
Copy link
Contributor

I would write "must support" instead of "should support", otherwise this sounds good. But I would tend not to the include the truncation example – I think the spec is perfectly clear even without it.

Also keep in mind that we repeat this language three times (for each of the relevant date/time types), so brevity is important. I suppose you mean to include the truncation example just once. Sure, that would be possible, but I don't think it's necessary.

@eksortso
Copy link
Contributor

It was intended to be succinct; I wrote it knowing that it would be repeated. We don't need to show the truncation example, but I do insist that we put emphasis on the word truncated.

I didn't intend to relax the millisecond precision requirement, which I'm almost certain some implementations will do anyway. I was only trying to avoid saying "must" twice, and I forgot the phrases in RFC 2119 that meant the same thing. We technically aren't using RFC 2119 now, but #870 may push us that way.

Let's return to the original wording somewhat for that first sentence.

Implementations are required to support at least millisecond precision. Additional digits of precision may be specified, but if they exceed the supported precision, then the extra digits must be truncated, not rounded.

Copy link
Contributor

@eksortso eksortso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toml.md
Here are the changes to clarify that the sub-millisecond precision is a requirement specifically for the implementations to support. (I tried to find a way to request explicit changes, but it seems I can't do this, for whatever reason. You'll have to compare your version and the version I attached here for the differences.)

Copy link
Contributor

@eksortso eksortso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @aduh95 for including the changes.

@eksortso
Copy link
Contributor

@pradyunsg This PR draws an important distinction and relieves some potential confusion. What are your thoughts?

Copy link
Contributor

@ChristianSi ChristianSi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT!

@eksortso
Copy link
Contributor

eksortso commented Apr 3, 2023

@pradyunsg This has been sitting dormant for four months. Does anything more need done?

@eksortso
Copy link
Contributor

@pradyunsg It's been six months. What are your thoughts on this? Can we merge it for v1.1.0?

@ChristianSi
Copy link
Contributor

Pleeeeese!

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.

4 participants