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

Serialize projectionDefinition to arrays #906

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 17, 2024

This is how it would look to serialize to arrays like color does.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.78%. Comparing base (e56dada) to head (d921832).

Additional details and impacted files
@@                           Coverage Diff                            @@
##           projection-expression-collecting-branch     #906   +/-   ##
========================================================================
  Coverage                                    92.77%   92.78%           
========================================================================
  Files                                          107      107           
  Lines                                         4736     4741    +5     
  Branches                                      1345     1345           
========================================================================
+ Hits                                          4394     4399    +5     
  Misses                                         342      342           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@birkskyum birkskyum changed the title Use arrays Serialize projectionDefinition to arrays Nov 17, 2024
}

static interpolate(from: string, to: string, t: number) {
return new ProjectionDefinition(from, to, t);
return new ProjectionDefinition(from, to, t).toJSON();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think interpolate should return the same type of object and not a string...

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay,

Just to clarify, this does return an array, not a string.

@HarelM
Copy link
Collaborator

HarelM commented Nov 17, 2024

I'm failing to see the motivation behind this PR to be honest...

@birkskyum
Copy link
Member Author

birkskyum commented Nov 17, 2024

@HarelM , thanks for being honest with it, I'm fine with either.

I do see that color does get serialized to array somehow (saw it in test/integration/expression/tests/interpolate-hcl/linear/test.json test), but I couldn't figure out where it happens.

My main motivation here was the comment above. I wanted to ensure that we made our choice on API design based, if we serialize to arrays or class objects, based on preference, instead of having technical limitations make the decision for us.

@birkskyum birkskyum closed this Nov 17, 2024
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.

3 participants