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.

Sign up for GitHub

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

Jump to bottom

[7.x][NMBObjCMatcher] Use NMBPredicate #546

Merged
merged 3 commits into from Jul 4, 2018
Merged

[7.x][NMBObjCMatcher] Use NMBPredicate #546

merged 3 commits into from Jul 4, 2018

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Jun 16, 2018 •
edited

This is internal non-breaking, implementations migrations for some public APIs.

@ikesyo ikesyo force-pushed the nmbobjcmatcher-nmbpredicate branch from 242bfbd to c3c452b Compare June 16, 2018 10:14
@ikesyo ikesyo requested a review from a team June 22, 2018 17:00
sharplet
sharplet reviewed Jun 23, 2018
View reviewed changes
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.

When you say "internal API migrations" I'm a little uncertain what you mean — aren't these public API? If you could elaborate on how this makes migration easier I'd love to understand.

Sources/Nimble/Matchers/BeAKindOf.swift Outdated
return try! beAKindOf(expected).matches(actualExpression, failureMessage: failureMessage)
@objc public class func beAKindOfMatcher(_ expected: AnyClass) -> NMBPredicate {
return NMBPredicate { actualExpression in
return try! beAKindOf(expected).satisfies(actualExpression).toObjectiveC()
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, can this closure be throwing? Would be nice to replace try! with try (although I'm not familiar with the possible failures here).

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's revisit that in a separate PR (targeting to master branch).

@ikesyo
Copy link
Member Author

ikesyo commented Jun 25, 2018 •
edited

Does non-breaking, implementations migrations for some public APIs make sense?

@ikesyo
Copy link
Member Author

ikesyo commented Jun 25, 2018

While those are public API surfaces, the point is to change those implementations (yes it's privete, implementation detail) to avoid using deperecated Matcher's matches API.

sharplet
sharplet reviewed Jun 25, 2018
View reviewed changes
Sources/Nimble/Matchers/BeGreaterThan.swift Outdated
@@ -32,10 +32,10 @@ public func > (lhs: Expectation<NMBComparable>, rhs: NMBComparable?) {

#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
extension NMBObjCMatcher {
@objc public class func beGreaterThanMatcher(_ expected: NMBComparable?) -> NMBObjCMatcher {
return NMBObjCMatcher(canMatchNil: false) { actualExpression, failureMessage in
@objc public class func beGreaterThanMatcher(_ expected: NMBComparable?) -> NMBPredicate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so from what I can tell NMBObjCMatcher and NMBPredicate are separate classes that both conform to NMBMatcher — so it should be non-source-breaking to replace NMBMatcher with NMBPredicate (more specific return type), but it's technically breaking to replace NMBObjCMatcher with NMBPredicate, because they don't share a common base class other than NSObject. (Let me know if I'm missing something.)

It's probably unlikely to be a big deal, but worth considering.

Copy link
Member Author

@ikesyo ikesyo Jun 25, 2018 •
edited

Choose a reason for hiding this comment

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

Hmm it's unfortunate that the NMBObjcMatcher are exposed here instead of NMBMatcher. I'm okay to exclude them in this PR.

@ikesyo ikesyo force-pushed the nmbobjcmatcher-nmbpredicate branch from c3c452b to 8df083c Compare June 26, 2018 00:14
@ikesyo ikesyo changed the title [NMBObjCMatcher] Use NMBPredicate for easier API migration [NMBObjCMatcher] Use NMBPredicate Jun 26, 2018
@ikesyo ikesyo requested a review from sharplet June 26, 2018 00:17
@ikesyo
Copy link
Member Author

ikesyo commented Jun 26, 2018 •
edited

#546 (comment)

Ok so from what I can tell NMBObjCMatcher and NMBPredicate are separate classes that both conform to NMBMatcher — so it should be non-source-breaking to replace NMBMatcher with NMBPredicate (more specific return type), but it's technically breaking to replace NMBObjCMatcher with NMBPredicate, because they don't share a common base class other than NSObject. (Let me know if I'm missing something.)

It's probably unlikely to be a big deal, but worth considering.

It turns out that actually those API changes were partially done in v7.0.1 (#428, 4f0dab8).

@ikesyo ikesyo changed the title [NMBObjCMatcher] Use NMBPredicate [7.x][NMBObjCMatcher] Use NMBPredicate Jul 1, 2018
@ikesyo ikesyo force-pushed the nmbobjcmatcher-nmbpredicate branch from 8df083c to cb5e28b Compare July 3, 2018 14:00
sharplet
sharplet approved these changes Jul 4, 2018
View reviewed changes
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.

Current state of this looks good to me 👍

@ikesyo
Copy link
Member Author

ikesyo commented Jul 4, 2018

Thank you so much!

@ikesyo ikesyo merged commit dc9b86e into 7.x-branch Jul 4, 2018
@ikesyo ikesyo deleted the nmbobjcmatcher-nmbpredicate branch July 4, 2018 17:30
@ikesyo ikesyo mentioned this pull request Jul 9, 2018
Merged
@ikesyo ikesyo mentioned this pull request Aug 8, 2018
Merged
Megal pushed a commit to Megal/Nimble that referenced this pull request Jul 31, 2019
@ikesyo
Merge pull request Quick#546 from Quick/nmbobjcmatcher-nmbpredicate
cc8dc9a 
[7.x][NMBObjCMatcher] Use NMBPredicate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharplet sharplet sharplet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ikesyo @sharplet