add categorical expression #97
Conversation
|
@greensopinion Added the requested changes. Should be ready for another review. |
| final property = json['property']; | ||
| if (property is! String?) { | ||
| logger.warn(() => | ||
| 'Malformed categorical expression, property field must be an optional String: $json'); |
There was a problem hiding this comment.
Rather than litter the code with logs/warnings, we rely on https://github.com/greensopinion/dart-vector-tile-renderer/blob/main/lib/src/themes/expression/parser/expression_parser.dart#L124 as a catch-all.
| if (propertyName?.isEmpty ?? true) return null; | ||
|
|
||
| if (!stops.length.isEven) { | ||
| context.logger.warn(() => 'Could not parse categorical stops: $stops'); |
There was a problem hiding this comment.
is this check better put in the parser?
There was a problem hiding this comment.
moved check to the parser
|
|
||
| if (input['zoom'] > context.zoom) break; | ||
| if (input['value'] == propertyValue) { | ||
| tmpResult = output; |
There was a problem hiding this comment.
should we short-circuit here?
There was a problem hiding this comment.
Could you give me a bit more informationen in how you want it to be changed?
If the zoom level of the stop is not valid (not an integer), we stop it gracefully. This never happens if the theme is not malformed but I added it for more type safety.
if (inputZoom is! int) continue;If the zoom level in the loop gets higher than the zoom level of the map, we can leave the loop because all stops with an higher zoom level are no candidates (this assumes that the list is ordered by the zoom level which should be the case).
if (input['zoom'] > context.zoom) break;There was a problem hiding this comment.
by short circuit, I mean should we return the value since the property is equal
| // legacy categorical expressions | ||
| // https://docs.mapbox.com/style-spec/reference/other | ||
| if (json['type'] == 'categorical') { | ||
| final property = json['property']; |
There was a problem hiding this comment.
would we get better code reuse by moving this inside the stops is List check below?
| @@ -12,3 +13,56 @@ class GetPropertyExpression extends Expression { | |||
| @override | |||
| bool get isConstant => false; | |||
| } | |||
|
|
|||
| /// A function that returns the output value of the stop equal to the | |||
There was a problem hiding this comment.
Unless I'm mistaken, this looks copied from https://docs.mapbox.com/style-spec/reference/other/
We can't have any code or docs copied from anywhere. Please remove the docs here, and instead link to the spec.
| // [2] default value | ||
| // [3:] pair of stops | ||
| if (json.length < 3 || json.length.isEven) { | ||
| throw Exception( |
There was a problem hiding this comment.
this library is intended to fail gracefully, we don't want to throw here
instead, return null and the library will log that the expression isn't supported
| final propertyName = this.propertyName?.evaluate(context); | ||
| if (propertyName == null) return defaultValue?.evaluate(context); | ||
|
|
||
| final propertyValue = context.getProperty(propertyName!); |
|
|
||
| if (input['zoom'] > context.zoom) break; | ||
| if (input['value'] == propertyValue) { | ||
| tmpResult = output; |
There was a problem hiding this comment.
by short circuit, I mean should we return the value since the property is equal
| final input = stops[i]; | ||
| final inputZoom = input['zoom']; | ||
| if (inputZoom is! int) continue; | ||
| if (input['zoom'] > context.zoom) break; |
This pull request adds support for categorical expressions.
Example JSON snippet for a categorical expression: