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

Fixes #820 - @turf/intersect #890

Merged
merged 5 commits into from
Aug 11, 2017
Merged

Fixes #820 - @turf/intersect #890

merged 5 commits into from
Aug 11, 2017

Conversation

stebogit
Copy link
Collaborator

@stebogit stebogit commented Aug 4, 2017

  • added truncate to inputs
  • added originally breaking test case
  • return Feature<null> if no geometry
  • remove extra JSON.stringify

Ref #820

@stebogit stebogit self-assigned this Aug 4, 2017
@@ -3,6 +3,7 @@ const fs = require('fs');
const test = require('tape');
const load = require('load-json-file');
const write = require('write-json-file');
var truncate = require('@turf/truncate');
Copy link
Member

Choose a reason for hiding this comment

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

poor lonely var... 😢 he feels old next to all his younger const siblings.

var a = reader.read(JSON.stringify(geom1));
var b = reader.read(JSON.stringify(geom2));
var a = reader.read(JSON.stringify(truncate(geom1)));
var b = reader.read(JSON.stringify(truncate(geom2)));
Copy link
Member

Choose a reason for hiding this comment

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

Since truncate does the clone operation, we can drop JSON.stringify from the mix.

if (intersection.isEmpty()) {
return undefined;
}
if (intersection.isEmpty()) return null;
Copy link
Member

@DenisCarriere DenisCarriere Aug 4, 2017

Choose a reason for hiding this comment

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

Weren't we going to return a Feature<null> instead of null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I didn't noticed your suggestion in the issue comment.

Copy link
Member

@DenisCarriere DenisCarriere Aug 5, 2017

Choose a reason for hiding this comment

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

No worries 👍

I think we are all in agreement that if we have no results in collection of Features, then we would have an empty FeatureCollection, if the output is a single Feature which would be undefined or null then we would output a Feature<null> that way things don't blow up when you chain TurfJS methods by sending a null as input to another module.

@DenisCarriere
Copy link
Member

I like the new bug-fix label 😄

@DenisCarriere DenisCarriere added this to the 4.7.0 milestone Aug 11, 2017
@DenisCarriere DenisCarriere merged commit dd4d365 into master Aug 11, 2017
@DenisCarriere DenisCarriere deleted the intersect-issue-820 branch August 11, 2017 14:00
@dpieri
Copy link

dpieri commented Oct 24, 2017

Shouldn't this have bumped the major version as it changed the API for turf.union? Any code that interpreted an undefined responses to mean there is no union breaks on this change.

@DenisCarriere
Copy link
Member

DenisCarriere commented Oct 25, 2017

@dpieri This fix actually got reverted, PR #952 & Issue #951

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