-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improvements to Tuples #19172
base: main
Are you sure you want to change the base?
Improvements to Tuples #19172
Conversation
bc8fa14
to
eff2d54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change of syntax of old code is making this PR harder to review. Maybe split the additions from the syntax updates.
|
||
object Tuple { | ||
/** A tuple with the elements of this tuple in reversed order added in front of `acc` */ | ||
inline def reverseOnto[This >: this.type <: Tuple, Acc <: Tuple](acc: Acc): ReverseOnto[This, Acc] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline def reverseOnto[This >: this.type <: Tuple, Acc <: Tuple](acc: Acc): ReverseOnto[This, Acc] = | |
inline def reverseOnto[This >: this.type <: Tuple, That <: Tuple](taht: That): ReverseOnto[This, That] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer acc/Acc here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely better than "taht" :p
@experimental | ||
object Helpers: | ||
/** A tuple with the elements of tuple `X` in reversed order added in front of `Acc` */ | ||
type ReverseOnto[X <: Tuple, Acc <: Tuple] <: Tuple = X match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type ReverseOnto[X <: Tuple, Acc <: Tuple] <: Tuple = X match | |
type ReverseOnto[X <: Tuple, That <: Tuple] <: Tuple = X match |
fromArray(xs, xs.length) | ||
|
||
/** Convert the first `n` elements of an array into a tuple of unknown arity and types */ | ||
def fromArray[T](xs: Array[T], n: Int): Tuple = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could compute the type (Any, Any, ..., Any)
if we have a known n
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could. Needs to be discussed, and can be in a separate PR. I don't think the functionality is so important in fromArray
, but having a constructor for "type level list of known size" might be useful elsewhere.
@@ -82,82 +81,126 @@ sealed trait Tuple extends Product { | |||
/** Given a tuple `(a1, ..., am)`, returns the reversed tuple `(am, ..., a1)` | |||
* consisting all its elements. | |||
*/ | |||
@experimental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually do not mix the stabilization of API with the addition of new ones.
* for which the given type level predicate `P` reduces to the literal | ||
* constant `true`. | ||
*/ | ||
inline def filter[This >: this.type <: Tuple, P[_] <: Boolean]: Filter[This, P] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline def filter[This >: this.type <: Tuple, P[_] <: Boolean]: Filter[This, P] = | |
@exerimental | |
inline def filter[This >: this.type <: Tuple, P[_] <: Boolean]: Filter[This, P] = |
I don't really have the bandwidth to do further changes here. Should someone else take this over? |
New methods: filter, indicesWhere, reverseOnto
eff2d54
to
254b72a
Compare
I broke it down to several commits. |
I will take this over. I will split it onto different PRs for each addition and add tests for each one. The first part is in #19183. |
Just be aware that the named tuples PR has the same series of commits. I would not like to have to get back to this and sort out again a huge amount of merge conflicts. I already spent an afternoon doing that yesterday. So let's get these things in as they are. |
This is superseded by the work we did on NamedTuples. @EugeneFlesselle but it might be useful as a starting point for your improvements. |
Indeed, there seems to be some overlap. I'll rebase everything we've got and hopefully we'll be able to improve theses tuples soo enough 🤞 |
Improve organization and doc comments