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

[ios, macos] Add match expressions support. #11464

Merged
merged 6 commits into from
Mar 26, 2018

Conversation

fabian-guerra
Copy link
Contributor

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

Fixes #11009

  • Support match expressions.
  • Support coalesce expressions.
  • Add tests.

@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
@fabian-guerra fabian-guerra added this to the ios-v4.0.0 milestone Mar 15, 2018
@fabian-guerra fabian-guerra self-assigned this Mar 15, 2018
@fabian-guerra fabian-guerra force-pushed the fabian-coalesce-match-operators-11009 branch from 290009d to 7c8614b Compare March 16, 2018 21:30
@fabian-guerra fabian-guerra requested a review from 1ec5 March 16, 2018 21:33
@@ -665,6 +704,26 @@ - (id)mgl_jsonExpressionObject {
[expressionObject addObject:obj.mgl_jsonExpressionObject];
}];
[expressionObject addObject:self.operand.mgl_jsonExpressionObject];
return expressionObject;
} else if ([function isEqualToString:@"mgl_matchWithOptions:default:"]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1ec5 do you think this new operand needs to be added as part of #11472

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, eventually everything here will need to be installed as aftermarket expressions. But we’d still need a selector-like function as backup, in case the approach in #11472 breaks for any reason.


[expressionObject addObject:[self.arguments[1] mgl_jsonExpressionObject]];
return expressionObject;
} else if ([function isEqualToString:@"coalesce:"]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this.

@fabian-guerra fabian-guerra force-pushed the fabian-coalesce-match-operators-11009 branch from ede1ef6 to e75bd22 Compare March 22, 2018 16:44

[expressionObject addObject:[self.arguments[1] mgl_jsonExpressionObject]];
return expressionObject;
} else if ([function isEqualToString:@"mgl_coalesce"]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function needs a stub implementation in Objective-C, even if it’s just an assertion like the one for -[NSPredicate(MGLAdditions) mgl_matchWithOptions:default:]. Even if we don’t fully implement these functions in Objective-C, the developer may expect to be able to call -expressionValueWithObject:context:. As long as the function name allows for the right number of arguments, we can then raise an exception instead of allowing NSExpression’s parser to segfault.

@@ -665,6 +704,26 @@ - (id)mgl_jsonExpressionObject {
[expressionObject addObject:obj.mgl_jsonExpressionObject];
}];
[expressionObject addObject:self.operand.mgl_jsonExpressionObject];
return expressionObject;
} else if ([function isEqualToString:@"mgl_matchWithOptions:default:"]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, eventually everything here will need to be installed as aftermarket expressions. But we’d still need a selector-like function as backup, in case the approach in #11472 breaks for any reason.

} else if ([function isEqualToString:@"mgl_matchWithOptions:default:"]) {
NSMutableArray *expressionObject = [NSMutableArray arrayWithObjects:@"match", self.operand.mgl_jsonExpressionObject, nil];
NSArray *optionsArray = self.arguments[0].constantValue;
for (NSDictionary<id, NSExpression*> *options in optionsArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike with coalescing, I think it could get quite inconvenient to have to specify all the cases for a match expression inside an aggregate expression. We can simplify the function name to mgl_match: by supposing a vararg method signature for the underlying Objective-C method:

- (id)mgl_match:(NSExpression *)firstCase, ...;

Then the arguments to this expression would be firstCase, firstValue, secondCase, secondValue, …, defaultValue.

(So far I’ve been taking cues from NSExpression’s built-in functions, not necessarily including all the nouns that an Objective-C method selector would, but still including prepositions.)

Copy link
Contributor

@1ec5 1ec5 Mar 23, 2018

Choose a reason for hiding this comment

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

Varargs are rare or nonexistent among NSExpression’s built-in functions, so when we implement the aftermarket version of this function, we should make it look like a control structure. Control structures like TERTIARY() don’t have selectors, so the developer wouldn’t expect to call -expressionValueWithObject:context: on them. Therefore, we could call the function MGL_MATCH: and not need to explicitly name the arguments or add colons for each one. The call site would look like MGL_MATCH(input, firstCase, firstValue, secondCase, secondValue, …, defaultValue).

} else if ([function isEqualToString:@"mgl_coalesce"]) {
NSMutableArray *expressionObject = [NSMutableArray arrayWithObjects:@"coalesce", nil];

for (NSExpression *expression in self.operand.constantValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a bit awkward to say FUNCTION({1, 2, 3}, 'mgl_coalesce'), since people normally think about coalescing an array, not telling the array to coelesce itself. Fortunately, built-in expressions don’t have operands (at least not publicly), so #11472 will implement an mgl_match: function that takes a single argument. But this implementation should stay in case the aftermarket expression mechanism fails in the future.

@fabian-guerra fabian-guerra force-pushed the fabian-coalesce-match-operators-11009 branch from 90f984c to 3daa566 Compare March 26, 2018 18:11
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