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

Implement allowNull options to @turf/meta fixes #946 #947

Closed
wants to merge 5 commits into from

Conversation

DenisCarriere
Copy link
Member

Implements: #946
Ref Issues: #940

JS Example

var features = turf.featureCollection([
  turf.point([26, 37], {foo: 'bar'}),
  turf.point([36, 53], {hello: 'world'}),
  turf.feature(null)
]);

turf.featureEach(features, {allowNull: false}, feature => {
  // Callbacks 2 features
});

turf.featureEach(features, {allowNull: true}, feature => {
  // Callbacks 3 features
});

// Backwards compatible with TurfJS v4.0
// allowNull defaults to `false`
// most common errors in TurfJS are related to null geometries being contained in FeatureCollections
turf.featureEach(features, feature => {
  // Callbacks 2 features
});

Up for debate

allowNull currently defaults to false, there could be an argument to set this default as true. My personal preference is allowNull=false.

@stebogit
Copy link
Collaborator

@DenisCarriere I'll just comment only on this one but it applies to the other method as well.
The tests you put in place will certainly prove me wrong, but how can this

turf.featureEach(features, {allowNull: false}, feature => {
  // Callbacks 2 features
});

be backward compatible and not breaking code based on the current version?

I thought this would have been the right order:

turf.featureEach(features, feature => {
  // Callbacks 2 features
}, {allowNull: false});

I'm probably missing something here.

@DenisCarriere
Copy link
Member Author

@stebogit Agreed it's a bit confusing having options, it's even more confusing when handling featureReduce since if the options are defined after the callback it's in the same position as the initialValue which is defined as any value.

🤔 I think I'm just going to drop this PR... no options in all of these Array.map & Array.reduce typed methods.

@DenisCarriere
Copy link
Member Author

Going to close this PR, doing this I found a few issues in the tests, I'm going a new PR related to only changes related to the test.js

@DenisCarriere DenisCarriere deleted the allowNull branch September 27, 2017 16:55
@DenisCarriere DenisCarriere modified the milestones: 4.8.0, 5.0.0 Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants