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

Expression kinds #5628

Merged
merged 3 commits into from
Nov 9, 2017
Merged

Expression kinds #5628

merged 3 commits into from
Nov 9, 2017

Conversation

jfirebaugh
Copy link
Contributor

Introduce the same sort of discriminated union as is used in native: an expression (or function, or generally, any property value) has one of the following kinds: constant, camera, source, composite.

This is a prerequisite for #2739 / #3044.


type ConstantExpression = {
kind: 'constant',
evaluate: (globals: GlobalProperties, feature?: Feature) => any
Copy link
Contributor Author

@jfirebaugh jfirebaugh Nov 8, 2017

Choose a reason for hiding this comment

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

It's not feasible yet, but I hope to be able to specialize the evaluate method appropriately for the various kinds in a later commit. For example, constant expression evaluation should require no arguments, and the Feature argument should not be optional for source and composite expression evaluation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once this happens, we'll need a way to heatmap-color.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

❤️


type ConstantExpression = {
kind: 'constant',
evaluate: (globals: GlobalProperties, feature?: Feature) => any
Copy link
Contributor

Choose a reason for hiding this comment

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

Once this happens, we'll need a way to heatmap-color.

type SourceExpression = {
result: 'source',
evaluate: (globals: GlobalProperties, feature?: Feature) => any,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about FeatureExpression? Seems more direct to me, but maybe there was a reason we opted for SourceFunction over FeatureFunction before...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5531 suggests DataExpression -- let's hash out terminology there. For now, I'm happy just getting JS consistent with current native terms. 😄

return isFeatureConstant ? {
functionType: 'constant',
layoutSize: layer.getLayoutValue(sizeProperty, {zoom: tileZoom + 1})
} : { functionType: 'source' };
}

// calculate covering zoom stops for zoom-dependent values
const levels = declaration.expression.zoomStops;
const levels = (declaration.expression: any).zoomStops;
Copy link
Contributor

Choose a reason for hiding this comment

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

This any cast is justified by the !declaration || declaration.isZoomConstant() check above, right? Let's include a line comment indicating that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. For #2739 this code is going to get rewritten in cases over expression.kind, without any casts required, so I'm just going to leave it for now.

@jfirebaugh jfirebaugh merged commit fea5d5f into master Nov 9, 2017
@jfirebaugh jfirebaugh deleted the expression-kinds branch November 9, 2017 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants