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/integer/long test does not seem relevant for specification compliance #154

Closed
cyyynthia opened this issue May 15, 2024 · 5 comments
Closed

Comments

@cyyynthia
Copy link

The test suite includes a test that validates if the parser is capable of parsing the largest 64-bit number. The specification does not mention such ability as mandatory: it uses the term SHOULD and requires implementations to throw an appropriate error if the number cannot be represented losslessly.

I do this in my own implementation which is limited to the maximum integer a 64-bit float can safely represent, however this causes the valid/integer/long to fail. I think this test is not relevant; or should consider an explicit error as a pass (and only fail if the parser returns a number that is not the one expected due to a potential truncation)

@arp242
Copy link
Collaborator

arp242 commented May 15, 2024

Came up previously at squirrelchat/smol-toml#11 (sort of)

@cyyynthia
Copy link
Author

It quite exactly came from here as I've been setting up toml-test as a CI job 😄

@arp242 arp242 closed this as completed in 0ce094c May 23, 2024
@arp242
Copy link
Collaborator

arp242 commented May 23, 2024

-int-as-float now automatically skips this test; I think that's the clearest solution, as it's only an edge case for this situation really. Even though int64 is optional, in practice everything seems to support it.

Looks like smol-toml does support ints on encoding? Supporting it only on encoding and not decoding is kind of a weird situation that's not really supported right now.

Let me know if you really need any more changes here, but I think this should be good, although it will still require some work on smol-toml.

@cyyynthia
Copy link
Author

Looks like smol-toml does support ints on encoding? Supporting it only on encoding and not decoding is kind of a weird situation that's not really supported right now.

smol-toml can distinguish between floats and ints by using bigints, they're always serialized as plain integers. The encoder test tries to use that to its advantage but it's not super pretty.

But while decoding a toml document it does not decode them as bigints since it's more natural to see them as plain numbers (and I was aiming for a friction-less experience for consuming TOML documents in JS). I do want to add an option for always decoding integers as bigint eventually (which will allow testing smol-toml's understanding of float vs integer, and would allow it to pass this test too as bigint can be as large as the system's memory allows).

I think that's the clearest solution, as it's only an edge case for this situation really.

I guess that works. I mostly pointed out as I feel like a parser that only strictly implements the specification should be able to pass the test suite, but I guess opting out is easy enough anyway.

@arp242
Copy link
Collaborator

arp242 commented May 24, 2024

a parser that only strictly implements the specification should be able to pass the test suite, but I guess opting out is easy enough anyway.

Opting-out of "implementations should [..]" behaviour is probably the right choice, IMHO. On a failing test you need to make a conscious decision, rather than "silently" skipping an optional test.

Especially since very few need to do so, as almost everyone implements int64.

Of course it should be easy to pass the test suite regardless of int64 support.

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

2 participants