You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When it comes to parsing, I made the choice of never generating Instant objects and always ZonedDateTime objects (in UTC if needed).
That decision could be reversed by changing the contents of the if (offset.toLowerCase() == "z") in temporal.ts.
Parsing will never generate Temporal objects unless temporal:true is passed, in which case it will never generate a TomlDate.
Some differences when it comes to serializing can be found by comparing the "stringifies dates properly" and "stringifies Temporal values properly" tests. I consider these differences to be acceptable (mainly, not showing a decimal part after the seconds when there is no nanoseconds).
In accordance to TOML spec, supported types are Instant, PlainDate, PlainDateTime, PlainTime, and ZonedDateTime.
Duration, PlainMonthDay and PlainYearMonth are not supported.
Instant will be serialized but then parsed as a ZonedDateTime (see previous section).
ZonedDateTime will only keep the offset information, and lose the time zone and calendar identities.
This code imports a polyfill as a test dependency, which I think is acceptable, but it also imports the polyfill in the main code which is evidently not OK.
After removing these imports, and assuming typescript is ignored, the parsing part won't error out unless the option is toggled to true, as all direct accesses to the Temporal object are either in typescript expressions or in the parseTemporal function body.
The serializing part will error out, as it tests instanceof with runtime access to the Temporal object, but it could be solved by something like the following code:
In other news, the exposed types for parsing become a mess, due to the overlapping combinations of "does it include bigints ?", "does it include TomlDate ?" and "does it include Temporal types ?".
There are now 11 types which is obviously too many to expose in my view, due to forwards-compatibility.
I had introduced a generic version of the TomlValue and TomlTable types, generic in the supported primitives, which was removed I believe for simplicity. Given the multiplicity of types here, I think it should be reintroduced.
When it comes to parsing, I made the choice of never generating Instant objects and always ZonedDateTime objects
That's good. It's a bit unfortunate that OffsetDateTime was dropped in favor of a hybrid ZonedDateTime that does both offset and properly timezoned dates in the Temporal API imho, since the two are very different, but oh well.
I consider [the differences when it comes to serializing] to be acceptable (mainly, not showing a decimal part after the seconds when there is no nanoseconds).
Sounds good to me too.
This code imports a polyfill as a test dependency
Given that the lib will wait for Temporal to land in an even-supported Node version, the polyfill will be temporary in all cases since we'll be able to drop it from the tests and rely on platform support.
In other news, the exposed types for parsing become a mess
Yeah that's always the catch when making precise types, exponential growth 😔
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is not meant to be merged in the short term.
When it comes to parsing, I made the choice of never generating Instant objects and always ZonedDateTime objects (in UTC if needed).
That decision could be reversed by changing the contents of the
if (offset.toLowerCase() == "z")intemporal.ts.Parsing will never generate Temporal objects unless
temporal:trueis passed, in which case it will never generate a TomlDate.Some differences when it comes to serializing can be found by comparing the "stringifies dates properly" and "stringifies Temporal values properly" tests. I consider these differences to be acceptable (mainly, not showing a decimal part after the seconds when there is no nanoseconds).
In accordance to TOML spec, supported types are Instant, PlainDate, PlainDateTime, PlainTime, and ZonedDateTime.
Duration, PlainMonthDay and PlainYearMonth are not supported.
Instant will be serialized but then parsed as a ZonedDateTime (see previous section).
ZonedDateTime will only keep the offset information, and lose the time zone and calendar identities.
This code imports a polyfill as a test dependency, which I think is acceptable, but it also imports the polyfill in the main code which is evidently not OK.
After removing these imports, and assuming typescript is ignored, the parsing part won't error out unless the option is toggled to true, as all direct accesses to the Temporal object are either in typescript expressions or in the parseTemporal function body.
The serializing part will error out, as it tests
instanceofwith runtime access to the Temporal object, but it could be solved by something like the following code:In other news, the exposed types for parsing become a mess, due to the overlapping combinations of "does it include bigints ?", "does it include TomlDate ?" and "does it include Temporal types ?".
There are now 11 types which is obviously too many to expose in my view, due to forwards-compatibility.
I had introduced a generic version of the TomlValue and TomlTable types, generic in the supported primitives, which was removed I believe for simplicity. Given the multiplicity of types here, I think it should be reintroduced.