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

SI-6064 Add method contains to Option. #886

Closed
wants to merge 1 commit into from
Closed

Conversation

soc
Copy link
Member

@soc soc commented Jul 11, 2012

The Option API more or less mirrors the Collection API,
but it seems that somehow this method has been forgotten.

Review: @axel22

@paulp
Copy link
Contributor

paulp commented Jul 11, 2012

LGTM. (How has this one not come up before...)

@soc
Copy link
Member Author

soc commented Jul 11, 2012

That's what I'm constantly wondering, too. :-)

@axel22
Copy link
Member

axel22 commented Jul 11, 2012

LGTM, very nice.

@adriaanm
Copy link
Contributor

from https://scala-webapps.epfl.ch/jenkins/view/pr-validators/job/pr-scala-testsuite-linux-opt/509/consoleText

/localhome/hudson/workspace/pr-scala-testsuite-linux-opt/test/files/run/t6064.scala [FAILED]

@soc
Copy link
Member Author

soc commented Jul 12, 2012

Ouch, looks like I forgot to commit the check file or something. :-/

The Option API more or less mirrors the Collection API,
but it seems that somehow this method has been forgotten.

Review: @axel22
@soc
Copy link
Member Author

soc commented Jul 12, 2012

The original issue was that I forgot to name the object Test, but I don't understand the build failure now:

[partest] No such file or class on classpath: Test
[partest] 
[partest] testing: [...]/files/run/t6064.scala                                  [FAILED]

It works fine on my machine:

./partest files/run/t6064.scala 

Testing individual files
testing: [...]/files/run/t6064.scala                                  [  OK  ]

All of 1 tests were successful (elapsed time: 00:00:05)

Any ideas?

@paulp
Copy link
Contributor

paulp commented Jul 12, 2012

It appears likely it does not see your update which renamed the object. Open a new pull request.

@adriaanm
Copy link
Contributor

PLS REBUILD ALL

let's see if the bot is willing first

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/518/

@soc
Copy link
Member Author

soc commented Jul 12, 2012

Btw, what do yo think about find?

@axel22
Copy link
Member

axel22 commented Jul 12, 2012

Isn't find already available through an implicit conversion on Option to Iterable?

@soc
Copy link
Member Author

soc commented Jul 12, 2012

Ooops, yes you're right. I played with reflection typeOf[Option[_]].members thinking about how to automate a diff between Option's and Collection's API, instead of looking in ScalaDoc and didn't try it. :-/

@paulp
Copy link
Contributor

paulp commented Jul 12, 2012

Why would you ever use "find" rather than "filter" ?

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/518/

@adriaanm
Copy link
Contributor

must this really go into 2.10? I thought we had agreed on critical bugs only (yet this is an improvement)

@soc
Copy link
Member Author

soc commented Jul 13, 2012

@adriaanm Hey Adriaan!
Is there anything I would have to do in the GitHub pull request UI to request the pull to a certain branch next time?
I didn't intend to target it specifically at 2.10, but after thinking a bit about it, I guess this would be my rationale for it:

It is not in line with "critical bugs only", but I tried to be as responsive as possible to an issue filed by a new, external bug reporter to prevent the "thanks for reporting, we fixed it, but you won't be able to use it until 2013/2014" scenario, but that of course doesn't matter much in the grand scheme of things.
My personal opinion is that it is a low-risk addition with a working test case fixing an earlier oversight in the library with no changes to the language, the type-system or the compiler.

@axel22
Copy link
Member

axel22 commented Jul 13, 2012

When submitting a pull request, there is a combobox at the top now with branches in the target repo.

@soc
Copy link
Member Author

soc commented Jul 13, 2012

Ah ok, thanks @axel22!

@adriaanm
Copy link
Contributor

Hi @soc, thank you for contributing so enthusiastically! :-)

In theory I agree with your reasoning. However, I feel we really need to hunker down and get this release done.

In the grand scheme of things I feel it's more important to get the 2.10 release out of the door in time (it's quite big as it is).
This can only happen if we strictly adhere to our critical bugfixes only policy.

I'm willing to make an exception for documentation, as there's no way that can cause regressions and, well, documentation is super important.

This could go into 2.10.1, which shouldn't be too far away.

@adriaanm adriaanm closed this Jul 13, 2012
@soc
Copy link
Member Author

soc commented Jul 13, 2012

No, problem! I'm not sure if merging with 2.10.x is allowed due to compatibility rules. Is compiling code with 2.10.1 and running it on 2.10.0 not supported (or was it only the other way around)?

@soc
Copy link
Member Author

soc commented Jul 18, 2012

@paulp Do you remember the exact rules for adding a method in minor releases? As far as I understand adding contains has to be deferred to 2.11 because otherwise code using contains compiled against 2.10.1 would crash when run with the 2.10.0 standard library.

Please let me know, so I can create a precise commit title for a fixed pull request against the appropriate branch.

Thanks!

@paulp
Copy link
Contributor

paulp commented Jul 18, 2012

I am not confident to say. I thought we were only protecting libs compiled against 2.10.0 working with 2.10.1, not the other way around, but I defer to @jsuereth.

@jsuereth
Copy link
Member

jsuereth commented Sep 5, 2012

This can be added in 2.10.1. (a) Option is a class and (b) We don't do forwards binary compatibility, just backwards. The latest scala release used for a library is the one required.

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