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

Convert ORKResultPredicate into a class extension of NSPredicate #754

Open
md0u80c9 opened this issue Jul 8, 2016 · 5 comments
Open

Convert ORKResultPredicate into a class extension of NSPredicate #754

md0u80c9 opened this issue Jul 8, 2016 · 5 comments

Comments

@md0u80c9
Copy link
Member

md0u80c9 commented Jul 8, 2016

I was initially looking at the code with regard to making some features Swift-friendlier.

When I looked at ORKResultPredicate.h, the thought occurred to me that the ORKResultPredicate class is actually just initialising a set of NSPredicates.

Therefore, I have a very simple suggestion - it will be an API change but hopefully one which makes things easier for everyone.

Why not make all the publicly exposed functions in ORKResultPredicate initialiser functions on a class extension to NSPredicate, and retire ORKResultPredicate altogether?

The function names would probably rename to initForTimeOfDayQuestionResult...

If we needed to maintain backwards compatibility for a bit we could just have a set of boilerplate code functions still residing in an ORKResultPredicate class whilst it was deprecated (not sure what the thoughts are re. deprecation of APIs in ORK).

If agreed this would be helpful, happy to generate the PR for it.

@md0u80c9
Copy link
Member Author

md0u80c9 commented Jul 8, 2016

I have created a pull request for this this evening. Instead of using ORKResultPredicate, we have now created a category on NSPredicate called ORKResultPredicate.

The syntax for creating predicates is tidier as a result - in Swift you generate predicates using NSPredicate(...) syntax; in Obj C you use [[NSPredicate alloc] initWith...] syntax which is much nicer.

It will cause very minor API breakage to existing apps who will need to convert from the old syntax to this one - however as this is a 10 character change or so as demonstrated in ORKSample and ORKTest I'm hoping this is acceptable for the improved code readability.

Best wishes,

Andrew

@md0u80c9
Copy link
Member Author

I've done further work on this over the weekend. The original pull request is named NSPredicate-Issue754, and should be ready for review.

Following this I've been trying to make a more Swift-friendly interface for NSPredicate. I have had to do this in Swift; this is work in progress but I've published it as NSPredicate-GenericFuncs (the pull request title gives the clue as to why the code is in Swift!) I'm hoping the use of Swift is acceptable in this context. I have published it, but it is NOT ready yet for formal review and inclusion in the codebase.

The Swift code for this feels cleaner to me - a much smaller set of convenience inits for min/max, expected, or matches. In Swift we have a very small number of convenience inits with much shorter syntax as we can use type inference.

For objective C, we will need to retain the old syntax - so I'm rebuilding the old functions to just call the new Swift functions. Unlike NSPredicate-Issue754, Objective C code wouldn't see API changes for this issue; Swift ones could use the old functions but would be advisable to change to the new syntax which is much shorter and extensible.

I am however struggling a little with the choice question results - opinions and advice welcome here.

  • Firstly, Bools or textual matches don't use a subquery, whereas single choice match does - so we need to use a separate function. Reluctantly I've therefore had to use a different parameter name for the resultSelector (choiceResultSelector) to distinguish between the different calls. Can anyone see a cleaner way to do this, preferably so that choice results could be passed to the same resultSelector: expected: convenience init?
  • Secondly, I have a type safety issue with choices. I had mistakenly assumed ChoiceResults are strings. They aren't forced to be - they can be NSNumbers, or indeed anything. For the predicate, we have to be able to convert this object into a string - do we just insist that this is sent as a string for comparison? Or is there a better way of ensuring type safety here whilst also being flexible?

@md0u80c9
Copy link
Member Author

I've now put three separate PRs relating to predicates up.

  • The first, PredicateTests, extends ORKTest to have a range of tests for predicates.
  • The second, NSPredicate-Issue754 simplifies the API to initialise predicates.
  • The third, NSPredicate-GenericFuncs is work-in-progress to develop simpler Predicate interfaces in Swift.

@md0u80c9
Copy link
Member Author

I've done some work on this today, effectively merging the latter two together, and making a number of tidy-ups. The objective C initialisers are marked as Obj C-only, and in Swift (2.2) we have a set of convenience inits utilising generics replacing all the initialisers. I've temporarily implemented NSDate as comparable in there; when Swift 3 is ready we can change to using Date which is Comparable by default.

@md0u80c9
Copy link
Member Author

md0u80c9 commented Jul 29, 2016

I've formally merged the two related PRs I've been working on here into a single PR named Predicates. I've left PredicateTests as a separate PR for now since it adds functionality to the Test suite. Predicates should be ready for review.

#763 #771

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

No branches or pull requests

2 participants