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

Reduce NMBObjCMatcher usages #600

Merged
merged 1 commit into from
Sep 18, 2018
Merged

Reduce NMBObjCMatcher usages #600

merged 1 commit into from
Sep 18, 2018

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Sep 6, 2018

See also #592.

@ikesyo ikesyo force-pushed the reduce-nmbobjcmatcher-usages branch from 803e746 to dbc91fa Compare September 6, 2018 15:26
@objc public class func beFalseMatcher() -> NMBMatcher {
return NMBPredicate { actualExpression in
let expr = actualExpression.cast { value -> Bool? in
guard let value = value else { return nil }
Copy link
Member Author

Choose a reason for hiding this comment

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

nil should not be converted to false here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, good catch. Should we add a test that covers this difference between beFalse() and beFalsy()? We're also accepting values that fail to cast to NSNumber, should we also return nil if the cast fails?

Copy link
Member Author

@ikesyo ikesyo Sep 18, 2018

Choose a reason for hiding this comment

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

That is already covered here:

Previously the behavior is ensured by NMBObjCMatcher(canMatchNil: false):

if !canMatchNil {
if try actualExpression.evaluate() == nil {
failureMessage.postfixActual = " (use beNil() to match nils)"
return false
}
}

Copy link
Member Author

@ikesyo ikesyo Sep 18, 2018

Choose a reason for hiding this comment

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

What I wanted to say is that the message

nil should not be converted to false here.

is only applicable to the new NMBPredicate impl in this PR, but not to the previous NMBObjCMatcher impl.

@@ -13,7 +13,6 @@ - (void)testPositiveMatches {
expect(@"hello world!").toNot(endWith(@"hello"));
expect(array).to(endWith(@2));
expect(array).toNot(endWith(@1));
expect(@1).toNot(contain(@"foo"));
Copy link
Member Author

Choose a reason for hiding this comment

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

Testing contain matcher in ObjCEndWithTest.m is odd (and the fact the test is passing is odd as well) 😛

@ikesyo ikesyo requested a review from a team September 7, 2018 15:56
Copy link
Contributor

@sharplet sharplet left a comment

Choose a reason for hiding this comment

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

One comment, but lgtm 👍

@objc public class func beFalseMatcher() -> NMBMatcher {
return NMBPredicate { actualExpression in
let expr = actualExpression.cast { value -> Bool? in
guard let value = value else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, good catch. Should we add a test that covers this difference between beFalse() and beFalsy()? We're also accepting values that fail to cast to NSNumber, should we also return nil if the cast fails?

@ikesyo ikesyo force-pushed the reduce-nmbobjcmatcher-usages branch from dbc91fa to 3ff206a Compare September 18, 2018 16:21
@ikesyo
Copy link
Member Author

ikesyo commented Sep 18, 2018

I will go ahead 🚀

Thanks for the review @sharplet!

@ikesyo ikesyo merged commit d53595b into master Sep 18, 2018
@ikesyo ikesyo deleted the reduce-nmbobjcmatcher-usages branch September 18, 2018 17:01
Megal pushed a commit to Megal/Nimble that referenced this pull request Jul 31, 2019
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.

2 participants