peterstace / simplefeatures

Simple Features is a pure Go Implementation of the OpenGIS Simple Feature Access Specification
MIT License
134 stars 19 forks source link

Function for NewCoordinates #595

Closed akhenakh closed 1 month ago

akhenakh commented 8 months ago

I run into a stupid bug because specifying Z in a NewPoint(geom.Coordinates{}) results in it defaulting to Type: geom.DimXY, it makes sense.

package main

import (
    "fmt"

    "github.com/peterstace/simplefeatures/geom"
)

func main() {
    p := geom.NewPoint(geom.Coordinates{
        XY: geom.XY{
            X: 10,
            Y: 11,
        },
        Z: 400,
    }).AsGeometry()

    fmt.Println(p.DumpCoordinates())
    fmt.Println(p.AsText())
}
{0 [10 11]}
POINT(10 11)

In the rest of the API, though, helper functions helps to remind you to specify the dimension like geom.NewSequence(seq, geom.DimXY).

I did not find any NewCoordinates() ?

peterstace commented 8 months ago

Yeah I see what you mean, thank you for reporting your experience. I think there are a few different options to make this sort of bug less likely:

Option 1

Add a geom.NewCoordinates function that takes a geom.CoordinatesType. We'd also have to make it accept the right number of float64 arguments. So maybe the function signature would be like geom.NewCoordinates(geom.CoordinatesType, xyzm ...float64). Examples of use:

It might be best if it panics if the geom.CoordinatesType argument doesn't match the number of args, which is the same as what geom.NewSequence does.

Option 2

Create new functions for each coordinates type:

Usage examples:

Option 3

The geom.Coordinates are mainly used for the creation of geom.Point values. So maybe we could just create new point constructors like this:

Usage would look like:

There are already "special" constructors for points, e.g. geom.NewEmptyPoint, so there is definitely precedence for this sort of thing.


I'm sort of leaning towards option 3 as the best, but would love to hear your thoughts @akhenakh.

akhenakh commented 8 months ago

Thanks for your answer!

I agree with you, option 3 looks nicer to use, but does not really align with the rest of functions where we need to pass a geom.DimXY as argument. So my preference is 3, but consistency tends more to 1.

Maybe both option 1 AND 3 is right? NewPoint just being an helper.

peterstace commented 1 month ago

Fixed using an approach most similar to "option 1" above (see https://github.com/peterstace/simplefeatures/releases/tag/v0.52.0).