Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios, macos] Change the format for case expressions to a flat structure. #11450

Merged
merged 4 commits into from
Mar 26, 2018

Conversation

fabian-guerra
Copy link
Contributor

@fabian-guerra fabian-guerra commented Mar 14, 2018

Fixes #11007

  • Add compact syntax.
  • Add multiple branch support.
  • Add tests.
  • Add iOS 8.x support.

@fabian-guerra fabian-guerra self-assigned this Mar 14, 2018
@fabian-guerra fabian-guerra added this to the ios-v4.0.0 milestone Mar 14, 2018
@fabian-guerra fabian-guerra added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS beta blocker Blocks the next beta release labels Mar 15, 2018
NSMutableArray *arguments = [NSMutableArray arrayWithObjects:self.predicate.mgl_jsonExpressionObject, self.trueExpression.mgl_jsonExpressionObject, nil];
NSMutableArray *arguments = [NSMutableArray arrayWithObjects:self.predicate.mgl_jsonExpressionObject, nil];

if (self.trueExpression.expressionType == NSConditionalExpressionType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A simple conditional would look like this:

// If conditional, then trueExpression, else falseExpression
TERNARY(conditional, trueExpression, falseExpression)

A nested conditional would look like this:

// If firstConditional, then firstExpression,
//   else if secondConditional, then secondExpression,
//     else falseExpression
TERNARY(firstConditional, firstExpression,
        TERNARY(secondConditional, secondExpression,
                falseExpression))

So it’s the falseExpression that we need to examine here, not the trueExpression.

NSExpression *falseExpression;
if (argumentObjects.count > 3) {
if ([argumentObjects[rightBranchIndex] isKindOfClass:[NSArray class]] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For #11007, we need to implement a new custom NSExpression function that can represent multiple branches. TERNARY() only supports “if…then”, and “else”, but we need “else if…then” as well.

If NSExpession were to support this feature natively, I would expect it to have initializers similar to +[NSDictionary dictionaryWithObjects:forKeys:] and +[NSDictionary dictionaryWithObjectsAndKeys:], in other words:

  • +[NSExpression expressionForTrueExpressions:forConditionals:falseExpression:]
  • +[NSExpression expressionForTrueExpressionsAndConditionals:falseExpression:]

We could implement these methods in this PR to help address #11016 and remove the single-conditional initializer in #11278 (comment).

Once #11015 is implemented, I think the underlying implementation could become clearer, something like MGL_TERNARY(firstConditional, firstExpression, secondConditional, secondExpression, …, falseExpression). In the meantime, the existing FUNCTION() syntax should suffice.

@fabian-guerra fabian-guerra force-pushed the fabian-conditionall-branching-11007 branch from 3f39c0c to 6e3a8fa Compare March 21, 2018 21:07
caseArray = [caseArray subarrayWithRange:NSMakeRange(0, caseArray.count - 1)];
}

for (int index = 0; index < caseArray.count; index++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

-[NSArray count] returns an NSUInteger, so to avoid the risk of overflow, index should be an NSUInteger.

@@ -258,6 +258,25 @@ - (id)mgl_jsonExpressionObject {
return [self valueForKeyPath:@"mgl_jsonExpressionObject"];
}

- (id)mgl_case {
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation essentially has the developer writing something like this:

[NSExpression expressionWithFormat:@"FUNCTION({firstConditional, firstExpression, secondConditional, secondExpression, …, falseExpression}, 'mgl_case')"]

It’s good that the arguments all sit side by side now instead of requiring nested TERNARY() expressions. However, placing all the arguments inside a single aggregate expression would be inappropriate, because case behaves like a control structure rather than a typical mathematical function.

Per #11450 (comment), I think this function should be used more like this:

[NSExpression expressionWithFormat:@"FUNCTION(firstConditional, 'mgl_case:', firstValue, secondConditional, secondValue, …, falseValue)"]

It looks weird, but we need an operand, and it’s natural for the first conditional to be that operand. This design would require a category method of NSPredicate with the following vararg signature:

- (id)mgl_case:(id)firstValue, ...;

As part of #11472, we can implement a control structure–like MGL_SWITCH: function that’ll have a call site like this:

[NSExpression expressionWithFormat:@"MGL_SWITCH(firstConditional, firstValue, secondConditional, secondValue, ..., falseValue)"]

@fabian-guerra fabian-guerra force-pushed the fabian-conditionall-branching-11007 branch from 6e3a8fa to 40b5edd Compare March 23, 2018 20:06
@fabian-guerra fabian-guerra force-pushed the fabian-conditionall-branching-11007 branch from 40b5edd to a832f82 Compare March 23, 2018 20:09
@fabian-guerra fabian-guerra merged commit dd301fb into release-boba Mar 26, 2018
@fabian-guerra fabian-guerra deleted the fabian-conditionall-branching-11007 branch March 26, 2018 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beta blocker Blocks the next beta release iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants