Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Cherry-pick Expression.serialize() #11312

Merged
merged 3 commits into from
Mar 1, 2018
Merged

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Feb 26, 2018

Cherry-pick of Expression.serialize() from #11156. This is needed for accessor support on Android for #10721. I recall iOS moving to the same approach (can't find the comment atm).

cc @ChrisLoer @1ec5

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Feb 26, 2018
@tobrun tobrun self-assigned this Feb 26, 2018
@ChrisLoer
Copy link
Contributor

The native ignore for #6160 (562abdd) probably shouldn't be cherry picked because that fix has gone in on the native side (although @ansis it looks to me like maybe that didn't make it into boba yet and probably should)

@brunoabinader is working on the build failure at #11316

@ChrisLoer
Copy link
Contributor

To get the build working, rebase this on top of release-boba now that #11322 has merged.

@1ec5
Copy link
Contributor

1ec5 commented Feb 27, 2018

I recall iOS moving to the same approach (can't find the comment atm).

#11254

Issue #10714
- Each expression stores its operator as a string, and default serialization is [operator, serialize(child1), ...]
- Custom implementations of `serialize` for Expression types that don't follow the pattern
- expression::Value -> mbgl::Value converter
- node_expression bindings to expose `serialize`
 - Round-tripping expressions through serialization and checking that outputs don't change
 - Checking expression serialization against expected value from fixture
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Opened #11344 as followup.

@jfirebaugh jfirebaugh merged commit 936d92a into release-boba Mar 1, 2018
@jfirebaugh jfirebaugh deleted the tvn-cp-serialize branch March 1, 2018 00:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants