Predicates

[md0u80c9]

Merges work from previous PRs on Predicates.

Using an extension ResearchKit predicates can now be built with initialisers built onto NSPredicate.

In Swift, use of generics allows a much simpler API.

[YuanZhu-apple]

@md0u80c9 It seems a major API revemp.
@rsanchezsaez Any comments?

[md0u80c9]

Hi @YuanZhu-apple - moving the predicates to be extensions of NSPredicate is clearly a significant API change (albeit hopefully one which is felt to be logical and simplifies predicate use).

The Swift API utilising generics is hopefully not considered a major API change in isolation - it could be applied to the existing ORKResultPredicate class rather than to NSPredicate if it was felt the Obj C API changes to the abolish ORKResultPredicates was not appropriate, and makes things easier for Swift developers.

[rsanchezsaez]

Sorry for my late reply.

I'm not too sure about the API change. ResearchKit's usage of predicates seems quite limited in scope (these predicates are very rarely going to be useful anywhere outside ResearchKit) so using convenience class methods feels like a good way to contain this and signify intent. Why do you feel that building directly through initializers simplifies usage?

Regarding moving some construction code to Swift, I'm good with that (in the long-term everything should be written in Swift!). If you refactor to remove the API change (or convince me otherwise! ;), I can review the Swift migration of the basic functionality to Swift.

[md0u80c9]

Hi @rsanchezsaez,

The reason for proposing the changes to the Obj C was the verbosity of the convenience classes, and the need for a custom ORKResultPredicate class. By extending Predicates with custom inits then there is one less class to remember, and a much more compact syntax to deal with.

The Swift change is however the more useful one overall: having a generic syntax removes the need to worry about types (which if we manage to replace a load of Obj C with Swift in future we could probably abstract away a lot of the types we see in the current syntaxing).

I was going to suggest I just re-write the Swift bit out for now as a new PR, and submit that. However, before I do I need a bit of clarity on what we're doing with the Swift bits. Do we focus on Swift 3 only, or do we need to support 2.3 too? Important because there is a Date in there which will be handled differently under the two versions. You can see for now I just extended NSDate to be comparable and equatable - on Swift 3 that won't be necessary.

[rsanchezsaez]

On second thought, perhaps you are right and this is indeed a nice improvement. On Objective-C the syntax was ok, but on Swift is quite bad:

let predicateGoodMood: NSPredicate = ORKResultPredicate.predicateForChoiceQuestionResultWithResultSelector(resultSelector, expectedAnswerValue: "good") 

Your proposal makes it certainly look better:

let predicateGoodMood = NSPredicate(choiceResultSelector: resultSelector, match: "good")

This looks great, I'm ok adopting the category syntax and dropping the builder class.

@YuanZhu-apple What do you think?

---

I have not fully reviewed this PR yet, but I noticed there are some merge conflict markers on ORKJSONSerializationTests.m, could you take a look, and update the PR to be mergeable against master?

Thanks!

---

As for Swift 2.3, I'll let Yuan answer that as well. In my opinion we're fine supporting Swift 3 only. ;-)

[md0u80c9]

Right - the commits this morning should now mean that this branch merges. I've removed the NSDate extensions to make NSDate Comparable and Equatable as I think that is now done out of the box. Should be OK for review now :-).

[md0u80c9]

Important development point: I've introduced a struct 'TimeOfDay' in there. Other areas of the ResearchKit API could do with migrating to using this in Swift rather than current Obj C methods. I haven't done this as part of this PR - but worth considering as a separate PR.

[md0u80c9]

@rsanchezsaez friendly ping on this :-).

[rsanchezsaez]

Thanks! I'll try to have a look in the coming days.

[rsanchezsaez]

@md0u80c9 Sorry for the delay! I'm only starting to review this now. :-)

Could you make sure the ResearchKit unit tests compile and pass? With this change the ORKTaskTests (which contain several predicate tests) fail to compile.

You can run the tests by choosing the ResearchKit target and doing Product -> Test. (Be aware that the ORKTest target contains a different unrelated set of tests.)

[rsanchezsaez]

Also, shouldn't we try using the Swift code under the hoods on ORKResultPredicate.m, rather than duplicating it on both sides? ;-)

[md0u80c9]

@rsanchezsaez: My preference would definitely be that the under the hood bit was all in Swift, but I wasn't totally sure what the policy was as to whether Swift was our 'preferred' language here, of equal status, or Objective C was the primary with Swift only used where it was needed for language-specific features - if that makes sense.

[rsanchezsaez]

I think that if the Swift predicate methods completely implement all the functionality previously provided by the ORKResultPredicate methods, then it's safe to delete the ORKResultPredicate.h/ORKResultPredicate.m and just call the Swift interface from Objective-C. My thoughts are that if we have the same functionality implemented in both sides, then we should just favor the Swift implementation, rather than maintaining both code-paths in parallel. (Less code to maintain = less possible bugs). :-)

I think that most of ResearchKit is in Objective-C for historical reasons. I think that if Apple started ResearchKit today, they'd use Swift (@YuanZhu-apple correct me if I'm wrong). For example, ORKSample being completely written in Swift is a strong indicator of this.

---

I have not looked into the Swift part of this PR deeply yet, I need to sit down and read the code. I'll let you know further thoughts when I do.

Some initial thoughts:

• How about making processORKResultSubpredicates just a private initializer of the NSPredicate extension as well, only to be called internally. Can this be done?
• I think the argument names would be clearer if they more plainly stated their intent (as it is on the ObjC side). E.g., minimum -> minimumExpectedAnswer; expected -> expectedAnswer; matches -> expectedAnswerStrings; patterns -> matchingAnswerPatterns, etc.

[md0u80c9]

Hi,

Right - corrected the Tests syntax - should be OK now (though I note there is a conflict with project.pbxproj to be sorted to bring the pull request up to date). I think I tried completely replacing the h and c files before but didn't quite get the syntax working as intended.

Re. Swift - if the intent is that we can just write new stuff in Swift that would be nice; and indeed there are chunks of code I've seen in RK I'd be tempted to submit PRs to convert to Swift just for code neatness, or better object typing benefits.

Read your suggestions:

• processORKResultSubPredicates - I'd need to look at that to see if it's possible. Think it should be...
• I disagree with the naming - if you don't mind me pointing you to WWDC 2016 Session 403 on 'Swift API Design Guidelines' I think the reduced verbosity matches exactly what they are describing.

[rsanchezsaez]

I think you forgot to push your unit test changes. ;)

I'll check the session you mention regarding naming conventions.

[umerkhan-apple]

Could you resolve merge conflicts? Thanks!

[rsanchezsaez]

@umerkhan-apple If you don't mind, I'd like to review this further when I have the time. I need to think about the API changes. :-)

[umerkhan-apple]

@rsanchezsaez Sounds good!

[umerkhan-apple]

@rsanchezsaez Did you get a chance to review the API changes? Also @md0u80c9 Would you be able to resolve the merge conflicts?

[md0u80c9]

Conflicts resolved. I'd be interested in thoughts re the Swift API. I think there are lots of areas where we could move RK to be more Swift-friendly (but each would be potentially API-breaking - moving towards more protocols and value types rather than the class hierarchies). It sort of depends what the vision of the team is in terms of what direction RK is going in.

[umerkhan-apple]

@md0u80c9 Thanks! Will wait for @rsanchezsaez to review the PR before merging it.

[rsanchezsaez]

Not yet, sorry, I'll try to have a look soon. ;-)