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

Fix Instant.parse succeeding even when seconds are omitted on the JVM and JS #370

Merged
merged 4 commits into from
May 13, 2024

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Mar 18, 2024

Fixes #369

@dkhalanskyjb dkhalanskyjb force-pushed the fix-omitting-seconds-on-instant-parsing branch from 1e5668c to e198569 Compare March 25, 2024 10:26
@ilya-g
Copy link
Member

ilya-g commented Apr 19, 2024

Why should we fix inconsistency between platforms in this direction and not the other way?
Could it be that users would expect string representations produced by Java Time entities be parseable with Instant.parse?

@dkhalanskyjb
Copy link
Collaborator Author

After an internal discussion, we decided to keep this implementation similar to Java, which intentionally rejects Instant values without the seconds specified.

@hrach
Copy link

hrach commented Apr 29, 2024

@dkhalanskyjb I'd love to see the reasoning behind this :) Could you share please?

@dkhalanskyjb
Copy link
Collaborator Author

dkhalanskyjb commented Apr 29, 2024

Sure, here are several arguments for always requiring seconds:

  • Not breaking consistency with Java without a good reason is already an argument in itself, and we didn't find a good reason.
  • Symmetry with Instant.toString(), which always emits seconds.
  • The conceptual model of Instant is that of a timezone-independent moment of time. When you're in a given time zone, even an implicit one, it makes sense to interpret 13:30 as 13:30:00.000000000, using common conventions. But an Instant can be interpreted in any time zone, including those with non-whole-minute offsets from UTC (like +01:23:45). If you receive a 2024-01-02T03:47+01:00, parse it as 2024-01-02T02:47:00Z, and then interpret it in the +01:23:45 offset, does it make sense that you have rounded the omitted seconds to zero, but now received a non-rounded wall-clock result back?

Counterarguments:

  • Instant would be able to parse the output of OffsetDateTime.toString(). This should be easy to implement even if the default parse doesn't support it: [iOS] Fix parsing string without seconds #375 (comment)
  • Instant would be able to parse the output of LocalDateTime.toString() if an offset is appended to it. But then, why not parse a LocalDateTime directly and then convert it to an Instant?

@dkhalanskyjb
Copy link
Collaborator Author

We decided to implement Instant.parse in a way that a sufficiently smart DCO would be able to strip away the parsing and formatting machinery if only default parse and toString were used thoughout. @ilya-g, what do you think about me doing this in a separate PR, so as not to block the release? AFAIK, this DCO doesn't even exist yet, so no one should be negatively affected by this PR.

@dkhalanskyjb dkhalanskyjb changed the title Fix Instant.parse suceeding even when seconds are omitted on the JVM and JS Fix Instant.parse succeeding even when seconds are omitted on the JVM and JS Apr 29, 2024
@dkhalanskyjb dkhalanskyjb added this to the 0.6.0 milestone May 3, 2024
@ilya-g
Copy link
Member

ilya-g commented May 6, 2024

what do you think about me doing this in a separate PR, so as not to block the release?

ok, let's leave a TODO

@dkhalanskyjb dkhalanskyjb force-pushed the fix-omitting-seconds-on-instant-parsing branch from e198569 to 94a2e0d Compare May 13, 2024 11:19
@dkhalanskyjb dkhalanskyjb force-pushed the fix-omitting-seconds-on-instant-parsing branch from 94a2e0d to f9df45b Compare May 13, 2024 11:49
@dkhalanskyjb dkhalanskyjb merged commit a100ce8 into master May 13, 2024
@dkhalanskyjb dkhalanskyjb deleted the fix-omitting-seconds-on-instant-parsing branch May 13, 2024 11:49
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.

Instant.parse(..) inconsistent between jvm and ios targets
3 participants