-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix OptionIdOps.some to always return Some #873
Conversation
|
||
test("OptionIdOps.some"){ | ||
val s: String = null | ||
s.some.contains(s) should === (true) |
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.
Due to NPE I wasn't able to write it as s.some should === (Some(null))
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.
Thanks, @vpavkin!
Would you mind leaving a comment in the code itself about why it's done this way?
Also, would you mind naming the test something that better identifies that it's checking for behavior when null
is passed in? Maybe something like test(".some with null argument still results in Some #871")
.
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.
Looks like Option.contains
isn't available on Scala 2.10
@ceedubs my bad. Added more comments and changed code to use |
Current coverage is
|
@@ -76,4 +76,12 @@ class OptionTests extends CatsSuite { | |||
isEq.lhs should === (isEq.rhs) | |||
} | |||
} | |||
|
|||
// a test for OptionIdOps.some to return Some even if the argument is null |
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.
This comment probably isn't really needed since the test name says basically the same thing.
👍 thanks a bunch, @vpavkin! |
updated comment :) |
👍 |
Fix OptionIdOps.some to always return Some
Fix #871