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

Duplicate conditions in case expression should be reported as invalid #6252

Closed
samanpwbb opened this issue Feb 28, 2018 · 2 comments
Closed
Labels
api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) needs discussion 💬

Comments

@samanpwbb
Copy link
Contributor

samanpwbb commented Feb 28, 2018

mapbox-gl-js version: 0.44.1

Steps to Trigger Behavior

A case expression like follows is valid:

[
  "case",
  ["has", "rank"],
  1,
  ["has", "rank"],
  1,
  0
]

But an expression like following throws an error here:

[
  "match",
  ["get", "rank"],
  100,
  1,
  100,
  1,
  0 
]

Expected Behavior

Both of the above expressions are invalid according to spec expression definitions.

Actual Behavior

Match expression with duplicate condition is invalid, but case expression with duplicate condition is valid.

Adding this extra validation would enable us to lean more on mapbox-gl-js's internal validation rather than rolling our own validation on top.

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Feb 28, 2018

This requires an equivalence relation on expressions. Is your expectation that two expressions are considered "the same" if and only if they are "deep equal" JSON structures? So for instance the following is allowed, despite being obviously equivalent in a different sense (same output on all possible inputs):

[
  "case",
  ["has", "rank"],
  1,
  ["all", ["has", "rank"], true],
  1,
  0
]

We could implement this, but I'm not totally convinced of its value, and it would be more expensive than what we do for match, the syntax of which is carefully chosen to make the validation cheap. (For case it would require either O(n^2) deep equality operations, or JSON.stringifying all the input conditionals.)

Do you know of other tools that provide this sort of warning for conditional expressions (if/else if)?

@mourner mourner added cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) api 📝 needs discussion 💬 labels Mar 15, 2018
@mourner
Copy link
Member

mourner commented Jul 25, 2018

Looks like we're not able to implement this in a performant way at runtime, so we'll have to lean on invalidation in style authoring tools like Studio.

@mourner mourner closed this as completed Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) needs discussion 💬
Projects
None yet
Development

No branches or pull requests

3 participants