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

valid/datetime/milliseconds nanosecond precision #91

Closed
moorereason opened this issue Oct 19, 2021 · 8 comments
Closed

valid/datetime/milliseconds nanosecond precision #91

moorereason opened this issue Oct 19, 2021 · 8 comments

Comments

@moorereason
Copy link
Contributor

The current test gives:

1987-07-05T17:45:56.6Z

and expects:

1987-07-05T17:45:56.600000Z

I expected nanosecond precision to be preserved and survive a round time without the value changing.

Should this behavior be enforced by toml-test or left as a implementation detail for the user to decide?

@jidicula
Copy link

jidicula commented Oct 20, 2021

Just as a note for anyone curious, the TOML v1.0.0 spec leaves details on fractional seconds beyond milliseconds (which are required) up to the implementation:

Millisecond precision is required. Further precision of fractional seconds is implementation-specific. If the value contains greater precision than the implementation can support, the additional precision must be truncated, not rounded.

@arp242
Copy link
Collaborator

arp242 commented Oct 20, 2021

I expected nanosecond precision to be preserved and survive a round time without the value changing.

.6Z and .600000Z are equivalent, right? Although the trailing zeros are a bit funky and probably shouldn't be there. Or am I not understanding how this works?

@jidicula
Copy link

jidicula commented Oct 20, 2021

I expected nanosecond precision to be preserved and survive a round time without the value changing.

.6Z and .600000Z are equivalent, right? Although the trailing zeros are a bit funky and probably shouldn't be there. Or am I not understanding how this works?

Not necessarily, it's a fine distinction that concerns the precision of the timekeeping device. If .6Z is treated as equivalent to .600000Z, this implies that the timekeeping device used for the timestamp is precise only to 10^-1 s when in fact it is precise to 10^-6 s.

@moorereason
Copy link
Contributor Author

Thanks @jidicula for reminding me of what the spec says.

@arp242,
I agree...this feels like a trick question. 😉 Why am I hesitant to agree that .6 and .600000 are equivalent?? It's a fraction of a second, so 6 tenths of a second. They're equal. I think my brain is working properly.

My question is whether the toml-test suite should fail on something the spec says is implementation dependent. How can we avoid false negatives?

@arp242
Copy link
Collaborator

arp242 commented Oct 20, 2021

Why am I hesitant to agree that .6 and .600000 are equivalent?? It's a fraction of a second, so 6 tenths of a second. They're equal. I think my brain is working properly.

I was confused about this as well. It's because "6" and "60" aren't the same when talking about seconds, minutes, or hours, so you're using two different notations. Also it's mixing base-60, base-10, and base-24 for double the fun. Stupid Babylonians and their base-60 counting system!

My question is whether the toml-test suite should fail on something the spec says is implementation dependent. How can we avoid false negatives?

Yes, I thought about this yesterday too; but couldn't really come up with a good answer. My best is a tests/valid/optional directory (or something to that effect) and put all the implementation-defined and/or optional stuff in there, and possibly skip it by default too (but how do you enable it then?) Not entirely sure yet.

At the very least this test should be split in to two tests, so it's testing one thing per test.

@arp242
Copy link
Collaborator

arp242 commented Oct 20, 2021

Another option is to add a suffix to every test; .e.g valid/datetime/nanoseconds-opt.toml. You can then easily skip all of them with -ignore *-opt (at least, I think that should work – need to check).

Either way, I think it's fine to have these kind of tests in here; it's even useful since library authors can then decide if they want to support it yes/no and how. Just need a way to clearly mark them and make it easy to skip all of them without having to specify every test individually.

@moorereason
Copy link
Contributor Author

I took a second look at this issue, and I think it's invalid.

pelletier/go-toml generates Go tests based upon the toml-test/tests/ files. At the time, go-toml was using a simple "haveJSON == wantJSON" style of comparison, but I doubt you want to support that use case.

We've since discovered (like with inf vs +inf) that toml-test uses a custom JSON comparison framework instead of a simple JSON string comparison.

@arp242 arp242 closed this as completed in 850bb72 Nov 24, 2021
@arp242
Copy link
Collaborator

arp242 commented Nov 24, 2021

I ended up just truncating these tests to millisecond precision; there's no telling what an implementation supports, and mucking about with an "optional" test directory gets pretty complicated when you consider all the different permutations.

Instead, I documented it in the README as "implementation-defined behaviour". There could perhaps be some other things in that section as well.

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

No branches or pull requests

3 participants