-
Notifications
You must be signed in to change notification settings - Fork 23
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
add categorical expression #97
base: main
Are you sure you want to change the base?
Conversation
@greensopinion Added the requested changes. Should be ready for another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few suggestions/questions inline:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this check better put in the parser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved check to the parser
|
||
if (input['zoom'] > context.zoom) break; | ||
if (input['value'] == propertyValue) { | ||
tmpResult = output; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we short-circuit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would we get better code reuse by moving this inside the stops is List
check below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is ! redundant?
|
||
if (input['zoom'] > context.zoom) break; | ||
if (input['value'] == propertyValue) { | ||
tmpResult = output; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use inputZoom here?
@josxha is this PR abandoned? |
@deckerst yes, it's currently abandoned. |
This pull request adds support for categorical expressions.
Example JSON snippet for a categorical expression: