search

andrewharvey / geojson-polygon-labels

Command line tool to generate point labels from GeoJSON polygons
ISC License
82 stars 16 forks source link

Fix geojson-stream integration #10

Closed jfrankl closed 5 years ago

jfrankl commented 5 years ago

@andrewharvey I'm brand new to JSON streams, so I apologize if this is an improper approach. This seems to work on my sample geojson multipolygon and polygon data. I would greatly appreciate your feedback.

I was able to recreate the issue you referenced https://github.com/andrewharvey/geojson-polygon-labels/issues/7#issuecomment-463830696 where exploding multipolygons did not work. The challenge was that when the labelFeature function received a single feature, it would sometimes result in multiple label features.

My approach: within labelFeature, push each label to an array of labeledFeatures. The labeledFeatures array gets returned to the through function. There, we loop through the labeledFeatures and run this.queue for each label.

andrewharvey commented 5 years ago

This looks good to me, I'll merge it in but hold off doing a release until we can do further testing

jfrankl commented 5 years ago

@andrewharvey sounds good to me! Can I be of help in testing? For example, if we need 5 GeoJSON that meet certain criteria, I'd be glad to gather or generate what is needed. I'm not sure what is useful, but if there is a task you could delegate to me, I'd be glad to take it on.

andrewharvey commented 5 years ago

For the tests I'd prefer as minimal GeoJSON as possible to carry out the test, input and output for:

If you wanted to put these together or any other tests together, that would be great.

jfrankl commented 5 years ago

@andrewharvey Sure, I'd like to take a shot at this. I don't have much experience writing unit tests, so I could use a little more direction, if you don't mind.

  1. When you say "I'd prefer as minimal GeoJSON as possible", what does that mean? That the test GeoJSON should be small/simple files, or that you are hoping to have a small number of test GeoJSON files?
  2. Any testing libraries or patterns you prefer? Can you point me to any tests you've written on similar projects that I might use as a reference for what you'd like to see?
andrewharvey commented 5 years ago

I opened two tickets #11 and #12 that are relevant here.

At the moment you'll see all the code is in bin/. Longer term it's a better approach if the cli code is split from the core labeling code, that would allow for proper unit testing of each method/function.

But until that refactor is in place, I'm okay with simpler testing approach is is just here's a sample input and here's the expected output, and it runs the cli over each of those.

  1. The GeoJSON should be small/simple files, at least to begin with, no limit on the total number of files. That way if there is an issue somewhere it's eaiser to notice and debug with simpler source data. I would just do a single rectangle to start with, then work up to a slightly more complex polygon just enough to show a difference between the labelling algorithms. Could also do two rectangles part of a MultiPolygon, and two rectangles part of a GeometryCollection.

  2. My experience is with https://www.npmjs.com/package/tape and that's what I'd use if we did #11 (eg. https://github.com/mapbox/geojson-area). In the mean time I'd just do it a a shell script to run the script over all the inputs and compare with outputs.