ewestern / geos

This is a Haskell binding to Geos, the open-source geometry library
MIT License
13 stars 9 forks source link

WKT writing and parsing #2

Closed jamiecook closed 7 years ago

jamiecook commented 7 years ago

Hi,

I'm trying to improve my haskell and evaluate the various offerings for geo in haskell at the same time. To do that I'm trying to add WKT support to your library... but I'm having trouble.

At first I thought it would accept EWKT (the postgis extension) so I gave it "SRID=4326;LINESTRING..." but that gave

Assertion failed: (0 != g), function GEOSGetSRID_r, file geos_ts_c.cpp, line 3466.
ParseException: Unknown type: 'SRID=4326;LINESTRING'

So I guess parsing is at least working, however when I remove the SRID component and just give the basic linestring, I get an error that looks more like a c exception...

Tests Serialization
  Parses a bytestring to a linestring
  Serializes a LineString into a bytestring

Test suite failure for package geos-0.1.0.0
    test:  exited with: ExitFailure (-11)
Logs printed to console

Given I'm still pretty new to haskell, I'm not sure if I'm doing something wrong - or it's just a c thing.

Any pointers? (pardon the pun)

jamiecook commented 7 years ago

Hmmm, so while I think I still have a problem in my impl, it seems that I forgot to get tests running locally before breaking them

image

When running with a stock checkout (on a mac with sierra) and geos 3.6.1 installed via brew image

ewestern commented 7 years ago

Hey there. Regarding the exit failure on the tests, as it looks like a memory error, it seems likely that there is a bug where a finalizer is being called on an object that has already been destroyed. It's been long enough since I've used this library that I'm not even sure that it wasn't there before. I'll try to track it down this week sometime. I'm not familiar enough with the the WKT spec for geos, so I'm not sure about the SRID parsing ... Is it possible that it doesn't parse the SRID from the string and you are just supposed to set it separately?

Thanks for doing this, by the way.

jamiecook commented 7 years ago

Fuck yes!!!!

So it looks like my test setup was heavily obscuring what was going on here... for some reason if the test failed, it was simply crashing out rather than displaying any information about the failing test.

For WKT writing that meant that because whitespace was wrong "LINESTRING (1,2)" <> "LINESTRING(1,2)" it blew up.

For WKT reading that meant that because my initial impl missed the SRID, it blew up.

This is the expected output (when those bugs are fixed)

image

If I introduce an error by setting the wrong srid in the reader code (note the 4327)

image

I would expect to see "Can Parse WKT [FAILED]" or something similar

jamiecook commented 7 years ago

So yes... I believe this is now good to merge

jamiecook commented 7 years ago

wrt to the WKT spec - SRID is not part of the spec but is included in the Extended WKT spec which is used by PostGIS.

In this PR I've just said that if you want to parse things that can't have an SRID embedded, you have to provide the SRID at parse time. The other idea I had would be to embed that directly in the reader creation (i.e. createWktReader :: Maybe Int -> Geos WktReader)

ewestern commented 7 years ago

Awesome, thanks!

jamiecook commented 7 years ago

woo hoo - my first Haskell PR is merged :)

Did you manage to replicate my finaliser crashing in the tests?