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

Fix format expression options validation #8339

Merged
merged 7 commits into from
Jun 13, 2019
Merged

Conversation

ahk
Copy link
Contributor

@ahk ahk commented Jun 11, 2019

This branch fixes format expression options validation. Previously any value literals used within fields of the options object would fail to validate, causing the style to fail to run. This solution chooses to recursively unbundle fields of options objects. Unbundling is an operation that coerces new Object(...) constructed primitives (String, Boolean, Number) into bare primitives. This makes little difference except to all our internal runtime type reflection during validation.

Closes #8271

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

@ahk ahk force-pushed the 8271-format-options-validation branch 3 times, most recently from 4bcd229 to c19aa19 Compare June 12, 2019 01:14
@ahk ahk changed the title 8271 format options validation Fix format options validation Jun 12, 2019
@ahk ahk changed the title Fix format options validation Fix format expression options validation Jun 12, 2019
@ahk
Copy link
Contributor Author

ahk commented Jun 12, 2019

Potentially we'd also like to pick up the missing web documentation for format expression options here. If anyone knows that we already documented the options but they got lost (gl-native and gl-js are feature split on text-color right now) I would be happy to be pointed to the documentation text to include in this fix.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

This is looking good. Could use a couple additional test cases for a format expression with a text-font override, and with invalid values or invalid overrides

test/unit/style-spec/fixture/text-field-format.input.json Outdated Show resolved Hide resolved
@ahk ahk merged commit 71b94f0 into master Jun 13, 2019
@mourner mourner deleted the 8271-format-options-validation branch August 1, 2019 11:46
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.

Validation of "format" expression fails when options are provided
3 participants