-
Notifications
You must be signed in to change notification settings - Fork 943
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 z-coordinate as input for @turf/turf-tin #772
Conversation
packages/turf-tin/bench.js
Outdated
|
||
global.points = JSON.parse(fs.readFileSync(__dirname+'/test/Points.geojson')); | ||
const points = JSON.parse(fs.readFileSync(__dirname+'/test/Points.json')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When refactoring, use the path.join()
method to make it friendly for cross platforms (Windows).
const path = require('path');
const points = JSON.parse(fs.readFileSync(path.join(__dirname, 'test', 'Points.json')));
packages/turf-tin/index.js
Outdated
@@ -39,24 +39,42 @@ var featurecollection = helpers.featureCollection; | |||
*/ | |||
module.exports = function (points, z) { | |||
//break down points |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add some simple validation to test the input is correctly a FeatureCollection.
if (points.type !== 'FeatureCollection') throw new Error('points must be a FeatureCollection');
if (z) point.z = p.properties[z]; | ||
if (z) { | ||
point.z = p.properties[z]; | ||
} else if (p.geometry.coordinates.length === 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good
packages/turf-tin/test.js
Outdated
const fs = require('fs'); | ||
const path = require('path'); | ||
const test = require('tape'); | ||
const tin = require('./index.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index.js
can be dropped.
const tin = require('./');
packages/turf-tin/test.js
Outdated
|
||
fs.writeFileSync(path.join(__dirname, 'test', 'Tin.json'), JSON.stringify(tinned)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving the outputs is very useful to see the final results, also saving the fixtures using .geojson
makes it easier to preview the GeoJSON directly in GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good!
Addresses #628 for
@turf/tin
. PR has the following edits:test.js
andbench.js
syntax to ES6.Please fill in this template.
npm test
at the sub modules where changes have occurred.npm run lint
to ensure code style at the turf module level.