jyutzler
commented
7 years ago Comments fall into the following categories
- [x] eliminate unneeded pointer receivers -> converted to regular receivers https://github.com/go-spatial/geom/pull/1#discussion_r139510658 https://github.com/go-spatial/geom/pull/1/commits/c23be62ff0bab21612df4e4600c93e45fb52adb2
- [x] non-concur on changes that prevented interfaces to be duck-typed -> added
Vertexes()to LineString andLinearRings()to Polygon to disambiguate https://github.com/go-spatial/geom/pull/1/commits/e3fcebb78ebb311b14c05595bad575aef86ed81a - [ ] add nil checks to pointer receivers -> not useful https://github.com/go-spatial/geom/pull/1#discussion_r139523056
- [ ] make some tests table driven -> not useful (yet) https://github.com/go-spatial/geom/pull/1#discussion_r139526893
- [ ] non-concur on adding Points() to Geometry -> see below
In what is currently in master, there are no methods on Geometry. It is unclear to me how this is useful. We already know these things are geometry-related by virtue of being in the geom package. If a function takes a Geometry as an argument but you have to type switch everything anyway, you might as well pass interface{} as an argument. Instead, let's make our interfaces be useful, with useful methods drawn from Simple Features, the spec this is derived from. We're talking about a small number of methods with well-defined implementations. We've already spent more time discussing this than the total amount of time it will take to develop all of these methods for all implementations of the interfaces.
Let me put this another way. If you don't give me a way to implement BBox in a reasonable way, I will not use this API because it doesn't actually help me. A switch statement in a util function is not reasonable because that function must have intimate knowledge of every type of geometry type, including ones not written yet. To me, dragging your feet on this for the sake of interface purity is not constructive.
This PR now contains a complete implementation (structs and associated tests) for all of the interfaces in geom.go. Minor changes were made to the interfaces to improve clarity and consistency.