michaelcurry commented 5 years ago

IO GeoJSON Support

Adding the ability to Read & Write GeoJson as an IO option.

GeoJSON Geometry Type	Read	Write
`FeatureCollection`	✓	✓
`Feature`	✓	N/A
`Point`	✓	✓
`MultiPoint`	✓	✓
`LineString`	✓	✓
`MultiLineString`	✓	✓
`Polygon`	✓	✓
`MultiPolygon`	✓	✓

NOTE: Z-Axis is supported

michaelcurry commented 5 years ago

Linked to Issue #13

BenMorel commented 5 years ago

Thanks for the PR! :+1: First remarks:

You don't need to split the reader into AbstractGeoJSONReader and GeoJSONReader; I've done this for WKT as there are 2 very close formats (WKT and EWKT) that share the same base code, but in the case of GeoJSON there is only one format so this should never be necessary.
I would not create a GeoJSONParser either; WKT requires a parser because we need to go through the document "word by word", but in the case of GeoJSON, it's valid JSON, that we can read directly inside the GeoJSONParser class.
I don't like the fact that GeoJSONParser decodes the JSON, then later creates a new instance of GeoJSONParser with a re-encoded JSON, this is not efficient; the document should be parsed once with json_decode(), and the Reader class should work on nested arrays taken from this document.
I've seen that you expect uppercase attributes, e.g. $geojson_array['TYPE']; I've seen lowercase examples myself, therefore I assume that keys are not case-sensitive; as such, we would need to strtolower() or strtoupper() everything before each comparison.

BenMorel commented 5 years ago

This looks very good so far! 👍 I will provide my comments inline (mostly nitpicks).

BenMorel commented 5 years ago

Something that would be nice also, it to provide an explanation to each "Invalid GeoJSON" exception, as there is no clue where the error lies at the moment.

michaelcurry commented 5 years ago

Something that would be nice also, it to provide an explanation to each "Invalid GeoJSON" exception, as there is no clue where the error lies at the moment.

Resolved in Commit: https://github.com/brick/geo/pull/14/commits/a3eda381fe0c7cb6e8fb619f3390f74641d02a9a

michaelcurry commented 5 years ago

@BenMorel I believe I hit everything in your review. Please let me know if there is anything else 👍

Also, Thanks for the Quick Review 🎊

michaelcurry commented 5 years ago

Looks like I broke the tests earlier. I have resolved the issue in commit https://github.com/brick/geo/pull/14/commits/150043e608991fb83ecaa7daee74e21c2456016f Apologies

michaelcurry commented 5 years ago

@BenMorel All Review Items have been resolved. Please take another look at the Pull Request and let me know if there is anything else :+1:

michaelcurry commented 5 years ago

@BenMorel I believe I hit all change requests. Please let me know if I missed anything and I will get a Commit in for change ASAP.

BenMorel commented 5 years ago

Looks all right to me! 👍

Final thoughts:

just to confirm, you finally decided to be strict on case sensitivity of values, e.g. not accept "POLYGON" instead of "Polygon"? Something I had in mind could be to allow the parser to be lenient on case sensitivity, by a constructor configuration: __construct(bool $lenient = false)
something useful to add to GeoJSONWriter::write() would be an optional bool $prettyPrint = false parameter, that would set the JSON_PRETTY_PRINT flag in json_encode(), to get a human readable output?

Note that these are not mandatory and can be added later, but I'd like to have your thoughts on this.

michaelcurry commented 5 years ago

just to confirm, you finally decided to be strict on case sensitivity of values, e.g. not accept "POLYGON" instead of "Polygon"? Something I had in mind could be to allow the parser to be lenient on case sensitivity, by a constructor configuration: __construct(bool $lenient = false)

Looking other a few of the implementations of GeoJSON, they seem to be pretty standard in Case Sensitivity. That being said, I do think there should be a way for the user to "opt-in" to case insensitive mode.

something useful to add to GeoJSONWriter::write() would be an optional bool $prettyPrint = false parameter, that would set the JSON_PRETTY_PRINT flag in json_encode(), to get a human readable output?

I agree! I could see where that would be useful. Did not cross my mind too be honest.

If these are not mandatory for the initial tag/release, I would like to complete these in a separate Pull Request. If you are ok with that, I can make new Issues and you can assign them to me?

BenMorel commented 5 years ago

Excellent work here, thank you! 👍

Let's do this, you can open pull requests to add the features above. No need to open issues, you can open pull requests only, they're redundant!

BenMorel commented 5 years ago

Released as 0.2.2 🎉

brick / geo

IO GeoJSON Support #14

IO GeoJSON Support