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

Add has / !has operators #15

Merged
merged 2 commits into from
May 3, 2016
Merged

Add has / !has operators #15

merged 2 commits into from
May 3, 2016

Conversation

lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Feb 22, 2016

ref mapbox/mapbox-gl-style-spec#364

This will require a minor version bump.

cc @mourner @jfirebaugh @ansis

Todo

@mourner
Copy link
Member

mourner commented Feb 22, 2016

👍

t.equal(f({properties: {foo: true}}), true);
t.equal(f({properties: {foo: false}}), true);
t.equal(f({properties: {foo: null}}), true);
t.equal(f({properties: {foo: undefined}}), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect this to be true.

@lucaswoj
Copy link
Contributor Author

This PR is blocked on a decision about how the has operator should handle "literally undefined", undefined, and null. In vector tiles, the three are one-in-the-same. In GeoJSON, the three could be differentiated.

Open questions:

  • Should has evaluate to true or false for null?
  • Should has evaluate to true or false for undefined?
  • Should has evaluate to true or false for "literally undefined"?
  • Will these decisions be consistent with GL Native?
  • Will these behaviours be predictable for a GeoJSON file converted to vector tiles serverside?
  • Will these decisions be consistent with future implementations of geojson-vt?

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Apr 27, 2016

Let's go with the definitions you've chosen:

  • has is false for undefined
  • has is false for "literally undefined"
  • has is true for null

We may add explicit null to a future VT spec. I think it's unlikely we'll ever add explicit undefined.

@arunasank
Copy link

👋 @lucaswoj - this is going to be super useful to find stats with https://github.com/mapbox/osm-tag-stats (mapbox/osm-tag-stats#2) . Will this get merged soon?

@lucaswoj
Copy link
Contributor Author

lucaswoj commented May 3, 2016

@arunasank I'll get this into the next release!

@lucaswoj
Copy link
Contributor Author

lucaswoj commented May 3, 2016

With humility and hindsight, I think @jfirebaugh's proposed definitions for undefined are best. This is ready for 👀 . I also added a commit with some minor feature filter cleanup.

}

function compareDescending(a, b) {
return a - b;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work for strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest commit

Copy link
Contributor

Choose a reason for hiding this comment

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

How so? I still see a - b which returns NaN for strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😞 It got lost in the rebase shuffle. 😄 Fixed now!

@jfirebaugh
Copy link
Contributor

With humility and hindsight, I think @jfirebaugh's proposed definitions for undefined are best.

Ah, ok, I'm fine with that too. 😄 For those following along, the behavior in question is "should has evaluate to true or false for a property that exists and whose value is undefined?" The behavior we're settling on is that has will have the same behavior as the Javascript in operator, evaluating to true in that case.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented May 3, 2016

Cleaned up the commit history and addressed #15 (comment). Ready to 🚢?

@jfirebaugh
Copy link
Contributor

🚢

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.

4 participants