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

Add Comparable.rangeTo() to ComparableRange() #45

Closed
wants to merge 1 commit into from

Conversation

yongjhih
Copy link
Contributor

@yongjhih yongjhih commented Jan 24, 2020

// `ComparableRange.contains()` equivalent to 
//
//   bool Comparable<T>.between(T first, T endInclusive) =>
//     first <= this && this <= endInclusive;
//
// NOTICE: it's not equivalent to `IntRange.contains()` for now unless inherits `ComparableRange.contains()`.
// ref. https://github.com/JetBrains/kotlin-native/blob/master/runtime/src/main/kotlin/kotlin/ranges/Ranges.kt#L41
expect(
    DateTime(1984, 11, 19).rangeTo(DateTime(2020, 1, 1).contains(DateTime(1984, 11, 19)),
    isTrue
);

Resolve #35

@passsy
Copy link
Collaborator

passsy commented Jan 26, 2020

I've seen this PR and I'm actively working on it. I need a bit more time to consider multiple things:

One big difference compared to the Kotlin implementation is that dartx ranges here aren't directed, therefore allow 1.rangeTo(2) as well as 2.rangeTo(1). The latter would be invalid in Kotlin because start always has to be smaller than end.

Your ComparableRange doesn't follow this principle and I have to checkout if it is possible and if it makes sense.

Also I have trouble wrapping my head around the equality operator. ComparableRange<int> and IntRange can't be equal because IntRange requires the step to be equal as well, which doesn't exists in a range. Ignoring the step would lead to a == b but b != a which is not how equals should be implemented.

Additionally I've discovered that the current IntRange has a bug in isEmpty.

  1. 1.rangeTo(1) is not empty because it contains the value [1]. Right now it returns []
  2. In kotlin isEmpty is only true when the range is invalid (2..1). But this is valid in dartx and contains [2, 1]. This means isEmpty should always return false.

I need more time to think if the idea of 2.rangeTo(1) to be valid ([2, 1]) was a good idea.

@yongjhih
Copy link
Contributor Author

yongjhih commented Jan 27, 2020

Yes, I've also noticed that IntRange behavior is different to Kotlin implementation.

I was thinking to add Int.downTo() and IntProgression, but I just wanted to make this PR simple, I didn't touch it in this PR. IMO, we can create another issue for that as well. (I've implemented it on my fork: yongjhih@2f72567 in the first place, I don't have too much time to negotiate the PRs, so I kept it)

To create a progression for iterating in reverse order, use downTo instead of .. when defining the range for it.

ref. https://kotlinlang.org/docs/reference/ranges.html#progression

We can see the following behavior in Kotlin:

println(2.rangeTo(1)) // 2..1
println(2.rangeTo(1).isEmpty()) // true, different to dartx
println(2.downTo(1).isEmpty()) // false (IntProgression/Iterable) that's what IntRange.rangeTo does currently.

println(2.rangeTo(1).contains(1)) // false
println(2.downTo(1).contains(1)) // true (IntProgression/Iterable) that's what IntRange.rangeTo does currently.
println(2.rangeTo(1).contains(2)) // false
println(2.downTo(1).contains(2)) // true (IntProgression/Iterable) that's what IntRange.rangeTo does currently.

@passsy passsy mentioned this pull request Apr 25, 2020
@passsy
Copy link
Collaborator

passsy commented Apr 26, 2020

Closed in favour of #74

@passsy passsy closed this Apr 26, 2020
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.

Add Comparable.between(T, T) extension method
2 participants