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

Make Optional an IterableOnce #1418

Merged
merged 12 commits into from
Nov 17, 2024
Merged

Make Optional an IterableOnce #1418

merged 12 commits into from
Nov 17, 2024

Conversation

guizmaii
Copy link
Member

@guizmaii guizmaii commented Nov 14, 2024

So that we can:

def get(a: A): Optional[B] = ...
val list: List[A] = ...
val result: List[B] = list.flatMap(get)

TODO:

  • Fix 2.12 compilation

ghostdogpr
ghostdogpr previously approved these changes Nov 14, 2024
Copy link
Member

@sideeffffect sideeffffect left a comment

Choose a reason for hiding this comment

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

I thought Option(al) as Iterable is controversial https://www.wartremover.org/doc/warts.html#option2iterable

But if you really know what you're doing and want it, then I'm not adamantly opposed.

@@ -13,7 +14,7 @@ import scala.language.implicitConversions
* The only difference between this type and [[scala.Option]] is that there is an implicit
* conversion defined from `A`` to `Optional[A]`, and from `Option[A]`` to `Optional[A]`.
*/
sealed trait Optional[+A] { self =>
sealed trait Optional[+A] extends IterableOnce[A] { self =>
Copy link
Member

Choose a reason for hiding this comment

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

Why not Iterable?

Copy link
Member

@ghostdogpr ghostdogpr Nov 14, 2024

Choose a reason for hiding this comment

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

The goal was to make a Chunk[Optional[A]] behave the same way as a Chunk[Option[A]] where we can just use flatten or flatMap to get rid of the empty values. And those two methods require an IterableOnce.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, but the question still stands. Wouldn't it be better to extend Iterable since we're changing things anyway? Maybe there are other use cases which won't do with mere IterableOnce. Or do you think it will be more complicated?

Copy link
Member Author

@guizmaii guizmaii Nov 17, 2024

Choose a reason for hiding this comment

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

Why not Iterable?

Because Option doesn't extend Iterable but IterableOnce 🤷‍♂️
I'm just copying Option code 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Than that should be good enough for us too :)

@ghostdogpr
Copy link
Member

Seems IterableOnce was added in 2.13, in 2.12 the signature of flatMap was:

final override def flatMap[B, That](f: A => GenTraversableOnce[B])(implicit bf: CanBuildFrom[List[A], B, That]): That

@guizmaii
Copy link
Member Author

guizmaii commented Nov 17, 2024

@ghostdogpr

Seems IterableOnce was added in 2.13, in 2.12 the signature of flatMap was:

Well, my solution is just to make it compile with Scala 2.12.
The .flatMap(optional) doesn't compile with Scala 2.12 but IMO that's good enough.
If someone complains, it'll be time to evaluate if the effort to implement it for Scala 2.12 is worth it.
I don't think anyone will complain

So that we can:
```
def get(a: A): Optional[B] = ...
val list: List[A] = ...
val result: List[B] = list.flatMap(get)
```
Copy link
Member

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

I think this is good enough, immediate benefits and minimal maintenance efforts.

@guizmaii guizmaii merged commit 760c31c into series/2.x Nov 17, 2024
21 checks passed
@guizmaii guizmaii deleted the iterable_Optional branch November 17, 2024 03:59
@sideeffffect
Copy link
Member

Nice, thanks guys 👏

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.

3 participants