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

Cherry pick 'collator' fixes #6598

Merged
merged 3 commits into from
May 2, 2018
Merged

Cherry pick 'collator' fixes #6598

merged 3 commits into from
May 2, 2018

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented May 2, 2018

Fixes issue #6592 -- don't use camel case in expression argument names. This is important to get into release-0.45 because after it's publicly released any further change would be a breaking change to the style-spec.

Also pulls in changes to avoid constant-folding of collator objects. Because the same collator can give different results between environments, constant-folding of expressions that rely on a collator is not appropriate (at least during serialization).

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

/cc @anandthakker @jfirebaugh @mapbox/studio @mapbox/maps-design

Although the results shouldn't change between evaluations, they may change between environments, so we don't want constant folding to make serializations that are inconsistent on different machines.
`CollatorExpression#serialize` was not previously used by the test suite because of constant folding, and was broken -- it needed to explicitly serialize its children.
We no longer constant-fold Collator expressions, so there's no way to create a literal holding a Collator.
@ChrisLoer ChrisLoer merged commit 6a457ed into release-0.45 May 2, 2018
@ChrisLoer ChrisLoer deleted the cp-collator-fixes branch May 2, 2018 21:00
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