Add summary

[albertteoh]

Description

Adds Summary() and String() functions to output a text summary of each geometry.

Check List

Have you:

• Added unit tests? Y

• Add cmprefimpl tests? (if appropriate?)

Related Issue

Fixes #384 and continued on from #393.

[peterstace]

So far this is looking pretty good! I only got through reviewing the geometry collection files, but I'll review the others soon.

[peterstace]

It would be good to recursively count the number of geometries here. c.NumGeometries() will just give the number of children in the geometry collection, which wouldn't be the total number.

E.g. consider the following geometry collection:

GEOMETRYCOLLECTION(
  GEOMETRYCOLLECTION(POINT(1 2), POINT(3 4)),
  GEOMETRYCOLLECTION(POINT(5 6), POINT(7 8))
)

c.NumGeometries() will give 2, but what we really want is 7 (the top level geometry collection, the 2 geometry collections under that, and the 4 points at the leaf level).

[albertteoh]

Nice catch! I was thinking, should NumGeometries return 7 in this case and rename the existing NumGeometries -> NumTopLevelGeometries which will return 2 in this case; what do you think?

[peterstace]

If we go with recursive counting and want to expose that to users (outside of the output of Summary()), I think it's better to create a new method (e.g. called NumTotalGeometries). There are other NumFoos methods on the other types, and they always have corresponding FooN methods, essentially allowing users to iterate through the children in a collection in a loop using for i := 0; i < g.NumFoos(); i++ { child := g.FooN(i); ... }. To keep geometry collections consistent with the other types, it's nice to keep NumGeometries and GeometryN as the "length" and "get" functions. The types having this pattern are:

• MultiPoint has methods NumPoints and PointN,
• MultiLineString has methods NumLineStrings and LineStringN,
• MultiPolygon has methods NumPolygons and PolygonN,
• GeometryCollection has methods NumGeometries and GeometryN.

That being said, in the Summary output, you are saying "child" geometries, so what you've got right now is correct from that standpoint. It's a fairly weird case to have deeply recursive geometry collections anyway (although it technically can happen). So on a bit more thought, I think what you've already got is ok (although I'm ok with you making it recursive if you want to, I'm happy to leave it up to you).

[peterstace]

What you've got will technically work ok, although there is a bit more of a concise and easier (for reading) way to put it. WKT is the lingua franca when describing geometries, and is generally used throughout the codebase (except for special circumstances for testing). The difference isn't so big in this file, but it would make a bit more of a difference in the others.

Suggested change

	geom.NewPoint(geom.Coordinates{XY: geom.XY{X: 0, Y: 0}, Type: geom.DimXY}).AsGeometry(),
	geomFromWKT(t, "POINT(0 0)"),

Btw, it's possible to create XYZ, XYZM, and XYM geometries too, e.g. POINT Z(0 1 2) and LINESTRING ZM(0 1 2 3,4 5 6 7).

[albertteoh]

That's much more readable, thanks for pointing this out. I'll see if it makes sense to do the same for the other geometry tests.

[peterstace]

Looks as though coordsType is unused.

[peterstace]

I finished reviewing the other files, everything is looking pretty good!

[peterstace]

Nice, this could be useful for other things as well 😄

[peterstace]

(commenting here since the file I would comment on for this change isn't in the diff)

The other thing that would be good to add is Summary() string and String() string on the Geometry type. This will unfortunately involve one of those nasty 7-case switch statements (at least until #399 is looked at).

This will actually make the testing a little bit easier, since you don't have to call the Summary methods directly on each concrete geometry. You can use something like:

wkt := "POLYGON((0 0,0 1,1 0,0 0))"
g := geomFromWKT(t, wkt)
got := g.Summary()
// check got vs want

Because you're operating on the generic Geometry type at that point, you can actually do the Summary tests cases for all 7 types all in the same unit test (since your test cases would just be string inputWKT and string wantSummary). It may also make sense to put these tests into geom/attr_test.go, which contains the other tests that calculate geometry "attributes" (in a sense, the summary string is a calculated "attribute" of a geometry).

[albertteoh]

i like the suggestion; i noticed I was repeating a very similar pattern on each geometry's test after adopting the WKT approach.

[albertteoh]

I've addressed your comments in 353c2ef; PTAL.

[peterstace]

Perfect! LGTM

[albertteoh]

Thanks for the review and guidance @peterstace!