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

Support two-argument shorthand for "match" expression #6964

Closed
wants to merge 1 commit into from

Conversation

jfirebaugh
Copy link
Contributor

Support two-argument shorthand for "match" expression. Fixes #6950.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

@jfirebaugh jfirebaugh requested a review from anandthakker July 13, 2018 21:56
@samanpwbb samanpwbb requested a review from emilymdubois July 13, 2018 22:15
@emilymdubois emilymdubois self-assigned this Jul 13, 2018
@nickidlugash
Copy link

@jfirebaugh Just wanted to check up on the status of this PR. Thanks!

@jfirebaugh
Copy link
Contributor Author

It's pretty much ready to go, I'm just not totally convinced it's worth it -- it's a modest syntax improvement that fixes an ergonomic papercut, but it adds a special case internally, and in similar-feeling situations that's been a source of future bugs and a potential snag to hit when making changes. My main concern is that the syntax doesn't round trip through the internal representation -- in native, if you serialize the result of getting an expression of this form from the runtime styling API, it'll use true, false. In mapbox/mapbox-gl-native#12415 (comment) @1ec5 details some of the nuances associated with that. We could introduce additional code so that it round trips, but then it's even more of a special case internally.

@samanpwbb
Copy link
Contributor

I'm just not totally convinced it's worth it -- it's a modest syntax improvement that fixes an ergonomic papercut, but it adds a special case internally, and in similar-feeling situations that's been a source of future bugs and a potential snag to hit when making changes.

This change also adds complexity and a special case to Studio. It'd be fine to support it, but if it makes any difference, I am a soft 👎 here.

@ryanhamley
Copy link
Contributor

@nickidlugash @samanpwbb How are we feeling about this change? The general consensus seems to be a resounding "Meh" but I'd like to make sure this doesn't just end up in purgatory so we should probably either merge it or close it. FWIW, @1ec5 said on Slack that he was not opposed to the change.

@nickidlugash
Copy link

If others are not sure that this is worth supporting, I'm fine with not moving forward with this. I've seen match statements trip up style authors, given the way data filtering is typically defined (both in other programs/languages, and in our previous filter syntax), but it's probably not a big deal. If and when Studio supports a UI for expressions-based filters, perhaps this is something that can be glossed over in the UI instead?

@ryanhamley
Copy link
Contributor

Closing since no one seems to be a strong yes on this feature

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.

6 participants