Skip to content

Added LocalTime.Formats.ISO_BASIC (ISO 8601 basic format) #518

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

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

Conversation

lucgirardin
Copy link

No description provided.

@dkhalanskyjb
Copy link
Collaborator

ISO 8601 specifies that the format for time-of-day includes a leading T. The extended format can omit it, but a basic representation of time-of-day must always include T. Therefore, the format you're proposing is not a valid basic ISO time-of-day, and should not be named ISO_BASIC.

@lucgirardin
Copy link
Author

You are right, thanks for spotting this! I believe the same should also apply to LocalTime.Formats.ISO. Should I attempt to change it as well?

@dkhalanskyjb
Copy link
Collaborator

I believe the same should also apply to LocalTime.Formats.ISO.

The extended ISO format is allowed to omit the T, and since the format without the T is vastly more useful, we use that, so no.

In fact, regarding usefulness: is LocalTime.Formats.ISO_BASIC useful at all? When researching how people used date-time formats, I haven't seen anyone defining this format on their own.

@lucgirardin
Copy link
Author

I find it very useful, for example if one need to have a timestamp stored in a file name, in my case weather-20250415T065500Z.parquet.

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

Got it! I don't see any problems with adding a LocalTime.Formats.ISO_BASIC if it is indeed useful, but by that logic, it also makes sense to add LocalDateTime.Formats.ISO_BASIC (which is the 20250415T065500 part in your message).

@@ -279,6 +279,23 @@ internal class LocalTimeFormat(override val actualFormat: CachedFormatStructure<
}

// these are constants so that the formats are not recreated every time they are used
internal val ISO_TIME_BASIC by lazy {
LocalTimeFormat.build {
char('T')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see internal val ISO_DATETIME: our format is case-insensitive.

*
* @sample kotlinx.datetime.test.samples.LocalTimeSamples.Formats.isoBasic
*/
public val ISO_BASIC: DateTimeFormat<LocalTime>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also add tests. Examples: LocalDateTimeFormatTest#testIso, LocalDateFormatTest#testBasicIso, etc.

/**
* ISO 8601 basic format.
*
* Examples: `1234`, `123456`, `123456.789`, `123456.1234`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These examples are not valid for this format.

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.

2 participants