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

Align match behavior with case/== #6684

Merged
merged 2 commits into from
May 18, 2018
Merged

Align match behavior with case/== #6684

merged 2 commits into from
May 18, 2018

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented May 16, 2018

Makes ["match", ["get", k], label, match, otherwise] equivalent to ["case", ["==", ["get", k], label], match, otherwise]. This changes the behavior of match expressions where the runtime type of the input does not match the type of the labels: previously such expressions produced a runtime type error and then fell back to the property default value; now they produce the fallback value from the match expression.

Fixes #6680.

cc @mapbox/studio @mapbox/maps-design @mapbox/ios @mapbox/android

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

@jfirebaugh
Copy link
Contributor Author

It doesn't look like our documentation was specific enough to cover this nuance, so it technically doesn't require an update. Does anyone think we should make it more specific?

Selects the output whose label value matches the input value, or the fallback value if no match is found. The input can be any string or number expression (e.g. ["get", "building_type"]). Each label can either be a single literal value or an array of values.

@jfirebaugh jfirebaugh requested a review from anandthakker May 16, 2018 21:57
@1ec5
Copy link
Contributor

1ec5 commented May 17, 2018

Yes, I think we should make the documentation more specific, assuming we don’t intend to revisit this decision anytime soon. Without additional documentation, I think some developers may still be initially surprised that ["match", ["get", k], label, match, otherwise] resolves to otherwise instead of match when k’s type differs from label’s.

I agree with this change, but the previous error message at least gave the developer a clue as to where the problem might lay. Without either the error message or documentation, the developer is left to guess that match fell through to the else case.

@anandthakker
Copy link
Contributor

anandthakker commented May 17, 2018

Agreed re: clarifying the docs. Draft:

Selects the output whose label value matches the input value, or the fallback value if no match is found. The input can be any expression (e.g. ["get", "building_type"]). Each label must either be a single literal value or an array of literal values (e.g. 'a', ['c', 'b']), and those values must be all strings or all numbers. (The values '1' and 1 cannot both be labels in the same match expression). Input values are matched against labels without doing any type conversions first, so if the input's type does not match that of the values, the "match"'s result will be the fallback value.

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.

LGTM!

return new Match((inputType: any), (outputType: any), input, cases, outputs, otherwise);
}

evaluate(ctx: EvaluationContext) {
const input = (this.input.evaluate(ctx): any);
return (this.outputs[this.cases[input]] || this.otherwise).evaluate(ctx);
const output = (typeOf(input) === this.inputType && this.outputs[this.cases[input]]) || this.otherwise;
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad we can't just use Map

@anandthakker anandthakker added the breaking change ⚠️ Requires a backwards-incompatible change to the API label May 17, 2018
Makes `["match", ["get", k], label, match, otherwise]` equivalent to `["case", ["==", ["get", k], label], match, otherwise]`. This changes the behavior of match expressions where the runtime type of the input does not match the type of the labels: previously such expressions produced a runtime type error and then fell back to the property default value; now they produce the fallback value from the match expression.
Copy link
Member

@tristen tristen left a comment

Choose a reason for hiding this comment

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

LGTM too!

We can now take advantage of the relaxed typing for == and match, which allows full-fidelity conversion of categorical functions. Previously, we were relying on the default value being substituted at evaluation time if the input was not of the expected type. Now, case/match can handle that.
@jfirebaugh
Copy link
Contributor Author

@anandthakker Want to review the conversion addition here?

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.

☺️ cc @samanpwbb

@jfirebaugh jfirebaugh merged commit a5a8dfe into master May 18, 2018
@jfirebaugh jfirebaugh deleted the fix-6680 branch May 18, 2018 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants