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

Local date range #189

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

Conversation

PeterAttardo
Copy link

This pull request enables Kotlin range and progression semantics with the LocalDate and DatePeriod classes. Its implementation borrows from the patterns used for IntRange.

Example:

val start = LocalDate.parse("2020-01-01")
val end = LocalDate.parse("2020-12-31")

// calls function for each day of the year in 2020
(start..end).forEach {
    processDayOf2020(it)
}

// calls function for each day of the year in 2020, ordered in reverse
(end downTo start).forEach {
   processDayOf2020(it)
}

// calls function for every other day of the year in 2020
(start..end step DatePeriod(days=2)).forEach {
   processDayOf2020(it)
}

@dkhalanskyjb
Copy link
Collaborator

We have a similar request, but for Instant: #34
Could you please describe why this is needed? Either in the issue I linked if your use cases are similar to the ones listed there, or in a new issue.

@boswelja
Copy link

@dkhalanskyjb I've had a use case for this, though it's more of a convenience having a dedicated LocalDateRange compared to a ClosedRange<LocalDate>. I'm building a calendar library that allows consumers to update a particular range of dates on the screen, the intended use case for this is to allow changing the rendered content in chunks.

A few other use cases off the top of my head:

  • Representing a selected date range
  • Using plus/minus + forEach in conjunction to iterate over a large range in chunks (granted this could be achieved by having a list of dates and chunking that, but if you've only got a start and end date this could be more efficient)

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.

core/common/src/DateTimePeriod.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/test/DateTimePeriodTest.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
@PeterAttardo
Copy link
Author

Something I came across that's not exactly an issue in the library, but is a usability question when using the library in IntelliJ that I'm wondering if there's a good answer for.

Because Kotlin automatically imports kotlin.ranges.* into every file, that means that the extension function fun <T : Comparable<T>> T.rangeTo(that: T): ClosedRange<T> is by-default available for all code. LocalDate implements Comparable<LocalDate> and so if you type the following:

(localDate1..localDate2).forEach {
    // some code in here
}

localDate1..localDate2 will not display an Unresolved reference warning due to a failure to import kotlinx.datetime.rangeTo, but will happily use kotlin.ranges.rangeTo and return a ClosedRange<LocalDate> instead. Because ClosedRange<LocalDate> is not LocalDateRange, all of our nice progression methods are unavailable, and we get an Unresolved reference warning on forEach instead.

This is easily solved by importing kotlinx.datetime.rangeTo, but IntelliJ is unable to recognize this as the solution, because from its perspective there is no issue resolving the rangeTo operator, just an issue with calling non-existing methods on ClosedRange<LocalDate>. This means users will have to be aware of this pitfall, and manually type in the import in every file they want to use the range operator with LocalDate.

@dkhalanskyjb
Copy link
Collaborator

@PeterAttardo, maybe this issue can be solved if rangeTo is a member method of LocalDate? Good job noticing this problem!

@PeterAttardo
Copy link
Author

@dkhalanskyjb I was hoping not to have to touch the LocalDate class, but that does seem like the cleanest solution. I have added the range operators to LocalDate, but because expect classes don't allow default implementations, I have moved the implementation logic into static methods in LocalDateRange. That can be cleaned up in the future if default implementations are ever allowed.

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.

Compilation fails for me, and when I fix it, the getSize test fails.

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.

Other than the small naming issue, I think it's good to go!

core/common/src/LocalDateRange.kt Outdated Show resolved Hide resolved
/**
* Returns true iff every element in [elements] is a member of the progression.
*/
override fun containsAll(elements: Collection<LocalDate>): Boolean = elements.all(::contains)
Copy link
Member

Choose a reason for hiding this comment

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

Due to Collection being a covariant type, we cannot trust the actual type of element in elements to be LocalDate here.
Please add a test checking e.g. (dateProgression as Collection<*>).containsAll(listOf(Any()). It should be false, but not fail.

Copy link
Author

Choose a reason for hiding this comment

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

Is this a deeper issue than at first appears? The signature of the methods implies no such unsafety, however after digging into it I can see that the methods in AbstractCollection are annotated with @UnsafeVariance, while the methods in UByteArray contain explicit type checks. I suppose it's a concession to Java compatibility where the contains methods take instances of Object instead of a typed parameter?

In any event, I'll copy the type check logic from UByteArray and add the test.

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.

4 participants