Enable Expression to be created without an operator #1855

pjleonard37 · 2023-01-18T23:49:57Z

Resolves #1571

This PR updates Expression, adding an init that does not require an operator. With this change, GeoJSONSource's clusterProperties will align with the style spec (see here). Following the style spec, a cluster property needs to be an array of two elements with several accepted formats:

["propertyKey" : [operator, [mapExpression]]] 
["propertyKey" : [[operator, ['accumulated'], ['get', propertyKey]], [mapExpression]]] 
["propertyKey" : [[reduceExpression], [mapExpression]]]

Currently, only the first format works, and not more complex options described in the style spec:

For more advanced use cases, in place of operator, you can use a custom reduce expression that references a special ["accumulated"] value, e.g.: {"sum": [["+", ["accumulated"], ["get", "sum"]], ["get", "scalerank"]]}

This PR allows for Expression creation without an operator so that the additional formats work. I additionally updated the Symbol Clustering Example to show how the clusterProperties API can work with these formats.

Pull request checklist:

Describe the changes in this PR, especially public API changes.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. Put tests in correct Test Plan (Unit, Integration, All)
- If tests were not written, please explain why.
Add documentation comments for any added or updated public APIs.
Add any new public, top-level symbols to the Jazzy config's custom_categories (scripts/doc-generation/.jazzy.yaml)
Add a changelog entry to to bottom of the relevant section (typically the ## main heading near the top).
Update the guides (internal access only), README.md, and DEVELOPING.md if their contents are impacted by these changes.
If this PR is a v10.[version] release branch fix / enhancement, merge it to main first and then port to v10.[version] release branch.

PRs must be submitted under the terms of our Contributor License Agreement CLA.

maios

can we also support decoding an operatorless Expression? Right now Expression decode requires the first element to be an Operator

Sources/MapboxMaps/Annotations/ClusterOptions.swift

Apps/Examples/Examples/All Examples/Annotations/PointAnnotationClusteringExample.swift

pjleonard37 · 2023-01-20T12:07:51Z

Tests/MapboxMapsTests/Style/ExpressionTests/ExpressionTests.swift

@@ -81,24 +79,6 @@ final class ExpressionTests: XCTestCase {
        XCTAssertThrowsError(try JSONDecoder().decode(Expression.self, from: data))
    }

-    // MARK: - Helpers
-    func verifyExpressionArgument(for expression: Expression, toMatch argument: Expression.Argument, at index: Int) {


I removed this helper function as it was only used once

pjleonard37 · 2023-01-20T12:54:30Z

Thanks for the reviews @maios & @zmiao! I've updated the code based on your comments.

codecov · 2023-01-23T22:07:21Z

Codecov Report

Merging #1855 (621e778) into main (4b54016) will increase coverage by 0.01%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             main    #1855      +/-   ##
==========================================
+ Coverage   89.06%   89.08%   +0.01%     
==========================================
  Files         242      242              
  Lines       15080    15094      +14     
==========================================
+ Hits        13431    13446      +15     
+ Misses       1649     1648       -1

Flag	Coverage Δ
integration	`56.67% <62.96%> (-0.02%)`	⬇️
unit	`83.62% <92.59%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ources/MapboxMaps/Annotations/ClusterOptions.swift	`100.00% <ø> (ø)`
Sources/MapboxMaps/Style/Types/Expression.swift	`81.76% <92.59%> (+0.33%)`	⬆️
...boxMaps/Annotations/OffsetGeometryCalculator.swift	`67.97% <0.00%> (-0.57%)`	⬇️
Sources/MapboxMaps/Offline/OfflineErrors.swift	`47.36% <0.00%> (+3.50%)`	⬆️
Sources/MapboxMaps/Style/SourceType.swift	`93.75% <0.00%> (+12.50%)`	⬆️

Sources/MapboxMaps/Style/Types/Expression.swift

Co-authored-by: Mai Mai <mai.mai@mapbox.com>

* Force previous fastlane version (#1848) * Fix issue with map spinning when setting destBearing and sourceBearing to 102 (#1793) (#1828) Co-authored-by: Niwaka <61189782+NiwakaDev@users.noreply.github.com> * Update submodules SSH key (#1854) * Enable CircleCI contexts for shared environment variables (#1852) * Use the latest v4 Slack Orb * Add 'Slack Orb' context for each job * MAPSIOS-556: Adopt shared context for CocoaPods trunk token * MAPSIOS-558: Adopt shared context for Fastlane Match password * MAPSIOS-557: Adopt shared context for SDK Registry token * MAPSIOS-555: Adopt shared context for Apple credentials/ASC token * Remove unused license template (#1857) * Fix missing contexts, reuse CircleCI token (#1859) * Enable GoogleCloud shared credentials (#1861) * Enable 'Retry on Failure' up to 3 times for all tests (#1860) * Add missing public mapbox token (#1863) * Run IntegrationTests even when unit tests failing (#1864) * Add Mapbox public token to unit tests (#1865) * Move mapbox token to build unit tests (#1866) * Add public Mapbox token to build unit test jobs * Revert "Add Mapbox public token to unit tests (#1865)" This reverts commit efba681. * Improve stability of attribution parsing (#1849) * Mapsios 567 mark no op and deprecate v10.11.0 properties (#1867) * Extract integration tests to the separate CircleCI job (#1870) * Rename job to build host tests * Rename job to build tests * Rename job to build firebase tests * Rename Build tests job * Run integration tests * Code coverage command * Enable codecoverage by default * Run integration tests on nightly bases * Utilize default values for run-integration-tests job * Remove unused arg * Enable Expression to be created without an operator (#1855) * Change clusterProperties to accept array * Break up expression and reassemble for clusterProperties * Adjust example so app can run * Introduce operatorless expression * Add additional tests, update comments * Enable operatorless encoding, add tests * Add property back, update filtering * Remove generated update * Fix argument and operator access * Update Sources/MapboxMaps/Style/Types/Expression.swift Co-authored-by: Mai Mai <mai.mai@mapbox.com> * Add changelog, update expression operator to simplify Co-authored-by: Mai Mai <mai.mai@mapbox.com> * Bump activesupport from 6.1.5 to 6.1.7.2 in /scripts/doc-generation (#1874) Bumps [activesupport](https://github.com/rails/rails) from 6.1.5 to 6.1.7.2. - [Release notes](https://github.com/rails/rails/releases) - [Changelog](https://github.com/rails/rails/blob/v7.0.4.2/activesupport/CHANGELOG.md) - [Commits](rails/rails@v6.1.5...v6.1.7.2) --- updated-dependencies: - dependency-name: activesupport dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update Maps versions (#1875) * Update CoreMaps and Common * Update license * Update changelog * Generate development DocC on PR merges (#1873) * Generate DocC * Upload DocC to the 'latest' * Fix test results collection * Debug * Extra push to force CI trigger * Last fixes * Code review fixes * Add 10.11.0-rc.1 changelog entry (#1876) * Update SDK version (#1877) * Add missing changelog for #1828 (#1880) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Patrick Leonard <pjleonard37@users.noreply.github.com> Co-authored-by: Niwaka <61189782+NiwakaDev@users.noreply.github.com> Co-authored-by: Roman Laitarenko <roman.laitarenko@mapbox.com> Co-authored-by: Mai Mai <mai.mai@mapbox.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix issue with map spinning when setting destBearing and sourceBearing to 102 (#1793) (#1828) Co-authored-by: Niwaka <61189782+NiwakaDev@users.noreply.github.com> * Enable CircleCI contexts for shared environment variables (#1852) * Use the latest v4 Slack Orb * Add 'Slack Orb' context for each job * MAPSIOS-556: Adopt shared context for CocoaPods trunk token * MAPSIOS-558: Adopt shared context for Fastlane Match password * MAPSIOS-557: Adopt shared context for SDK Registry token * MAPSIOS-555: Adopt shared context for Apple credentials/ASC token * Remove unused license template (#1857) * Fix missing contexts, reuse CircleCI token (#1859) * Enable GoogleCloud shared credentials (#1861) * Enable 'Retry on Failure' up to 3 times for all tests (#1860) * Add missing public mapbox token (#1863) * Run IntegrationTests even when unit tests failing (#1864) * Add Mapbox public token to unit tests (#1865) * Move mapbox token to build unit tests (#1866) * Add public Mapbox token to build unit test jobs * Revert "Add Mapbox public token to unit tests (#1865)" This reverts commit efba681. * Improve stability of attribution parsing (#1849) * Mapsios 567 mark no op and deprecate v10.11.0 properties (#1867) * Extract integration tests to the separate CircleCI job (#1870) * Rename job to build host tests * Rename job to build tests * Rename job to build firebase tests * Rename Build tests job * Run integration tests * Code coverage command * Enable codecoverage by default * Run integration tests on nightly bases * Utilize default values for run-integration-tests job * Remove unused arg * Enable Expression to be created without an operator (#1855) * Change clusterProperties to accept array * Break up expression and reassemble for clusterProperties * Adjust example so app can run * Introduce operatorless expression * Add additional tests, update comments * Enable operatorless encoding, add tests * Add property back, update filtering * Remove generated update * Fix argument and operator access * Update Sources/MapboxMaps/Style/Types/Expression.swift Co-authored-by: Mai Mai <mai.mai@mapbox.com> * Add changelog, update expression operator to simplify Co-authored-by: Mai Mai <mai.mai@mapbox.com> * Bump activesupport from 6.1.5 to 6.1.7.2 in /scripts/doc-generation (#1874) Bumps [activesupport](https://github.com/rails/rails) from 6.1.5 to 6.1.7.2. - [Release notes](https://github.com/rails/rails/releases) - [Changelog](https://github.com/rails/rails/blob/v7.0.4.2/activesupport/CHANGELOG.md) - [Commits](rails/rails@v6.1.5...v6.1.7.2) --- updated-dependencies: - dependency-name: activesupport dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update Maps versions (#1875) * Update CoreMaps and Common * Update license * Update changelog * Generate development DocC on PR merges (#1873) * Generate DocC * Upload DocC to the 'latest' * Fix test results collection * Debug * Extra push to force CI trigger * Last fixes * Code review fixes * Add 10.11.0-rc.1 changelog entry (#1876) * Update SDK version (#1877) * Add missing changelog for #1828 (#1880) * Skip codecov for Xcode 12.5.1 (#1879) * Replace test skips with failures (#1862) * Couple of fixes for release workflow (#1883) * Add basic signposts for rendering tracing (#1818) * Add basic signposts for rendering tracing * turn on signposts in tests Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Patrick Leonard <pjleonard37@users.noreply.github.com> Co-authored-by: Niwaka <61189782+NiwakaDev@users.noreply.github.com> Co-authored-by: Roman Gardukevich <roman.gardukevich@mapbox.com> Co-authored-by: Roman Laitarenko <roman.laitarenko@mapbox.com> Co-authored-by: Mai Mai <mai.mai@mapbox.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ivan Persidsky <ivan.persidskii@mapbox.com>

pjleonard37 force-pushed the MAPSIOS-534-api-cluster-properties branch from 217dc59 to 24583be Compare January 19, 2023 22:45

pjleonard37 changed the title ~~Updated GeoJSONSource clusterProperties to align with the style spec~~ Enable Expression to be created without an operator Jan 19, 2023

pjleonard37 marked this pull request as ready for review January 19, 2023 23:26

pjleonard37 requested a review from a team as a code owner January 19, 2023 23:26

pjleonard37 requested a review from zmiao January 19, 2023 23:27

maios requested changes Jan 20, 2023

View reviewed changes

Sources/MapboxMaps/Annotations/ClusterOptions.swift Show resolved Hide resolved

Sources/MapboxMaps/Annotations/ClusterOptions.swift Outdated Show resolved Hide resolved

zmiao reviewed Jan 20, 2023

View reviewed changes

Apps/Examples/Examples/All Examples/Annotations/PointAnnotationClusteringExample.swift Show resolved Hide resolved

pjleonard37 commented Jan 20, 2023

View reviewed changes

pjleonard37 requested a review from maios January 20, 2023 12:54

pjleonard37 force-pushed the MAPSIOS-534-api-cluster-properties branch from d4c7fe6 to 058dd20 Compare January 20, 2023 16:37

evil159 reviewed Jan 24, 2023

View reviewed changes

Sources/MapboxMaps/Style/Types/Expression.swift Outdated Show resolved Hide resolved

maios reviewed Jan 24, 2023

View reviewed changes

Sources/MapboxMaps/Style/Types/Expression.swift Outdated Show resolved Hide resolved

maios approved these changes Jan 24, 2023

View reviewed changes

pjleonard37 and others added 11 commits January 24, 2023 12:30

Change clusterProperties to accept array

b627639

Break up expression and reassemble for clusterProperties

70ffd96

Adjust example so app can run

a9e7a5f

Introduce operatorless expression

24716ad

Add additional tests, update comments

0ac9502

Enable operatorless encoding, add tests

0e3d796

Add property back, update filtering

1399b2c

Remove generated update

8cb37cc

Fix argument and operator access

f8c3137

Update Sources/MapboxMaps/Style/Types/Expression.swift

e82d059

Co-authored-by: Mai Mai <mai.mai@mapbox.com>

Add changelog, update expression operator to simplify

621e778

pjleonard37 force-pushed the MAPSIOS-534-api-cluster-properties branch from d8fe5de to 621e778 Compare January 24, 2023 17:31

pjleonard37 merged commit f99d7b0 into main Jan 24, 2023

pjleonard37 deleted the MAPSIOS-534-api-cluster-properties branch January 24, 2023 19:24

pjleonard37 mentioned this pull request Jan 24, 2023

clusterProperties does not fully support Style Specification. #1571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Expression to be created without an operator #1855

Enable Expression to be created without an operator #1855

pjleonard37 commented Jan 18, 2023 •

edited

Loading

maios left a comment

pjleonard37 Jan 20, 2023

pjleonard37 commented Jan 20, 2023

codecov bot commented Jan 23, 2023 •

edited

Loading

Enable Expression to be created without an operator #1855

Enable Expression to be created without an operator #1855

Conversation

pjleonard37 commented Jan 18, 2023 • edited Loading

Pull request checklist:

maios left a comment

Choose a reason for hiding this comment

pjleonard37 Jan 20, 2023

Choose a reason for hiding this comment

pjleonard37 commented Jan 20, 2023

codecov bot commented Jan 23, 2023 • edited Loading

Codecov Report

pjleonard37 commented Jan 18, 2023 •

edited

Loading

codecov bot commented Jan 23, 2023 •

edited

Loading