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

Option zip should return Option #8394

Closed
scabug opened this issue Mar 12, 2014 · 15 comments
Closed

Option zip should return Option #8394

scabug opened this issue Mar 12, 2014 · 15 comments

Comments

@scabug
Copy link

scabug commented Mar 12, 2014

zip in the Option docs says:
zip[B](that: GenIterable[B]): Option[(A, B)]

but it acutally returns an Iterable[A, B]. I don't think it's a documentation bug, as returning an Option would make sense.

@scabug
Copy link
Author

scabug commented Mar 12, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8394?orig=1
Reporter: scalauserxxi (xiu)
Affected Versions: 2.10.3
See #8391

@scabug
Copy link
Author

scabug commented Apr 28, 2014

@gourlaysama said:
It is a scaladoc bug in the way the usecase (fake) signature of zip is generated:

If you look at the full signature in the Scaladoc for Option (expand the zip method in the doc and then expand the "fullSignature" section at the bottom), you will see that there are no Option type involved. And there cannot be since Option does not implement zip, Iterable does (and there is an implicit conversion from Option to Iterable).

@scabug
Copy link
Author

scabug commented Dec 8, 2014

Markus Klink (justjoheinz) said:
Given that we can zip all kinds of things, and get a Thing[(a,b)] this behavior is at least inconsistent. Would be nice if Option would implement zip properly.

@scabug
Copy link
Author

scabug commented Jan 18, 2015

Naftoli Gugenheim (naftoligug) said:
Forget about the documentation, why can't it return an Option? Isn't it just a matter of adding a zip method to Option?

@scabug
Copy link
Author

scabug commented Mar 16, 2015

Heikki Vesalainen (hvesalai) said:
+1 on zip returning Option instead of Iterable

@scabug
Copy link
Author

scabug commented Apr 7, 2015

Mehmet Ali Gözaydın (kubudi) said:
I opened a PR for this: #4433
I would be happy to hear from you

@scabug
Copy link
Author

scabug commented Jun 4, 2015

Vera Salvisberg (vsalvis) said (edited on Jun 4, 2015 9:40:02 AM UTC):
Updated PR by Mehmet Ali Gözaydın ready to be merged: scala/scala#4437

@scabug
Copy link
Author

scabug commented Nov 9, 2015

@SethTisue said:
4437 was replaced with scala/scala#4836

but, I'm afraid the Scala team doesn't consider this issue addressable in 2.12, given our 2.12 compatibility promises; maybe for 2.13? see my comments at scala/scala#4836 (comment)

@scabug
Copy link
Author

scabug commented Oct 13, 2016

@SethTisue said (edited on Oct 13, 2016 5:38:29 AM UTC):
It's unclear now whether this a ticket about adding Option.zip specifically, or about doing something about the Scaladoc issue more generally (which affects other collections methods, see linked issues), or both. Let's be careful not to close this until either both issues are fixed, or a separate ticket for one has been created.

@scabug
Copy link
Author

scabug commented Oct 13, 2016

Heikki Vesalainen (hvesalai) said:
I think the description of this issue answers the question above: " I don't think it's a documentation bug, as returning an Option would make sense.", i.e. this issue is about Option.zip not returning Option (which would make sense).

@ashawley
Copy link
Member

ashawley commented Feb 6, 2018

Fixing this would take marshaling the old patch scala/scala#4433 new patch scala/scala#6491 by Dale Wijnand that adds Option.zip and works on 2.13 and includes tests.

Is this a good first issue?

dwijnand added a commit to dwijnand/scala that referenced this issue Apr 5, 2018
@dwijnand dwijnand self-assigned this Apr 5, 2018
@dwijnand dwijnand added the has PR label Apr 5, 2018
dwijnand added a commit to dwijnand/scala that referenced this issue Apr 5, 2018
dwijnand added a commit to dwijnand/scala that referenced this issue Apr 5, 2018
dwijnand added a commit to dwijnand/scala that referenced this issue Apr 5, 2018
dwijnand added a commit to dwijnand/scala that referenced this issue Apr 5, 2018
@dwijnand dwijnand removed their assignment Apr 10, 2018
@dwijnand dwijnand removed the has PR label Apr 10, 2018
@SethTisue
Copy link
Member

Dale isn't working on scala/scala#6491 anymore, he says. Would someone else like to take it?

I've removed "good first issue" since there is now enough accumulated history and discussion on this to be potentially intimidating to a newcomer. Regardless, it's not a big change and shouldn't be hard for someone to pick up, assuming there aren't new objections to what seems to me (knock on wood) like emerging consensus on the PR about what the right design is.

@SethTisue
Copy link
Member

(And I've redone the labels to show that this is now about changing Option for 2.13, not about fixing Scaladoc. Someone might like to open a new ticket on the Scaladoc angle on this.)

@lrytz lrytz modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 23, 2018
dwijnand added a commit to dwijnand/scala that referenced this issue Apr 24, 2018
@NthPortal
Copy link

scala/scala#6491 seems to be moving again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants