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

Remove interpolate-projection #901

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Nov 13, 2024

Launch Checklist

@birkskyum I took a stab at using the interpolate for projection instead of introducing a new interpolation expression.

I'm still looking into understanding how color names are being parsed, but in general, I think this looks like a good direction.
Let me know what you think.

[
"linear"
],
[
"zoom"
],
0,
"vertical-perspective",
["literal", ["vertical-perspective", "vertical-perspective", 1]],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how I made the test work - calling literal on the project array allows converting the array to a Projection.
I'm not sure if this is great, would love to see what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it also works with regular strings when the interpreter knows which type it should output (projection in this case).

Copy link
Member

@birkskyum birkskyum Nov 13, 2024

Choose a reason for hiding this comment

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

regular strings?

I'd like to be able to define

10,
"vertical-perspective"
12, 
"+proj=merc +a=6378137 +b=6378137 +lat_ts=0 +lon_0=0 +x_0=0 +y_0=0 +k=1 +units=m +nadgrids=@null +wktext +no_defs +type=crs"

Is that possible?

instead of having it twice

12,
["+proj=merc +a=6378137 +b=6378137 +lat_ts=0 +lon_0=0 +x_0=0 +y_0=0 +k=1 +units=m +nadgrids=@null +wktext +no_defs +type=crs", "+proj=merc +a=6378137 +b=6378137 +lat_ts=0 +lon_0=0 +x_0=0 +y_0=0 +k=1 +units=m +nadgrids=@null +wktext +no_defs +type=crs", 1]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, yes, other tests are doing exactly that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there's any validation on the strings right now though... But I'm sure it won't be a big issue to add validation...

Copy link
Member

Choose a reason for hiding this comment

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

there is no validation since I took out the .presets / .projections - it's for later.

What's special about this test, that makes it need the ["mercator", "mercator", 1] when the other tests doesn't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing, I just wanted to see that literal supports projection casting.

Copy link
Collaborator Author

@HarelM HarelM Nov 13, 2024

Choose a reason for hiding this comment

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

To be honest, I started with that to see that this would work and afterward saw that regular strings also works and left it as is.

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (107bd0e) to head (72bf603).

Files with missing lines Patch % Lines
src/expression/index.ts 60.00% 2 Missing ⚠️
src/util/projection.ts 77.77% 2 Missing ⚠️
Additional details and impacted files
@@                             Coverage Diff                             @@
##           projection-expression-collecting-branch     #901      +/-   ##
===========================================================================
- Coverage                                    92.63%   92.56%   -0.07%     
===========================================================================
  Files                                          107      107              
  Lines                                         4749     4761      +12     
  Branches                                      1346     1352       +6     
===========================================================================
+ Hits                                          4399     4407       +8     
- Misses                                         350      354       +4     

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

@HarelM
Copy link
Collaborator Author

HarelM commented Nov 13, 2024

Edit: I think it is working as expected now according to the tests.

@HarelM HarelM requested a review from birkskyum November 13, 2024 12:34
@birkskyum
Copy link
Member

birkskyum commented Nov 13, 2024

I like it a lot! Nice that it's going away the interpolate-projection.

If we in any way can find a way to have just [step, [zoom], number, string, number, string] I think it would be worthwhile.

I imagine it's possible right now to do:

[step, [zoom], number, Projection, number, Projection]

... and then have a Projection.parse("mercator") ? It's not too bad, considering it's not repeated, but I'm thinking there was to be a way we can make this work [number, string, number, string].

@HarelM
Copy link
Collaborator Author

HarelM commented Nov 13, 2024

I can merge this to the other branch and you could test this. I think it should work, shouldn't it?

@HarelM
Copy link
Collaborator Author

HarelM commented Nov 13, 2024

[number, string, number, string] is not an expression that is supported and I don't want to invent more expressions. So I think this is a good middle ground.

@HarelM
Copy link
Collaborator Author

HarelM commented Nov 13, 2024

Feel free to merge when you think it's OK.

@birkskyum birkskyum merged commit 5d25160 into projection-expression-collecting-branch Nov 13, 2024
5 checks passed
@birkskyum birkskyum deleted the interpolate-expression-with-projection branch November 13, 2024 14:14
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