WIP: Decode Point and MultiPoint

[Hoovs]

Add support for Point and MultiPoint

[Hoovs]

@gdey @ARolek here is just a quick first pass at multi/point. Since it is my first real PR I had 3 questions.

1) The == in test file fails on comparing objects. Wondered what your thoughts were on making equals() methods for the types or if you had an idea on how to compare these other than ==? Or if for some reason I am using == wrong.

2) Were either of you thinking of a different way to do this? I am trying to figure out ways to tokenize for poly etc so i will expand the regex as I look at the types.

3) Are you ever planning to support Z/M for WKT? Looking at the point class you have it as [2]float64 so in order to put in a Z it would take a decent refactor. Same for M-cords

If you could just give it a quick glance I would appreciate any input before I churn on to long.

Thank you

[Hoovs]

Actually i spent some more time thinking on it and I have a 4th question. Why aren't encode and decode struct methods of the type of object they are working on? Are you both ok if i extend them to include a decode and equals method?

I see the pro of having all this code in WKT, imports in 1 place. All the logic to encode the stuff is there as well. But if you had it as part of the interface you could just call it without the switches and just call gg.encode(). For decode you need the switch still to find the object but then you can just take the () and pass that to the object as needed to make itself.

Comes down to how you think the design should flow I think. Open to either idea.

[ARolek]

@Hoovs thanks for the first pass. Responses to your questions:

1. Can you point to the like you're referencing? Maybe use reflect.DeepEqual()?
2. I think @gdey has some ideas on how he wanted some of the parsing to work for WKT so let's wait for his response.
3. We have been considering Z&M but as of current the geom package supports on 2 dimensions. We're getting more pressure to support Z&M so I would consider it a TODO now until we reconsider the core types

Why aren't encode and decode struct methods of the type of object they are working on? Are you both ok if i extend them to include a decode and equals method?

Are you talking about adding methods to the core types in the geom package? I would prefer to keep the encoders / decoders in sub packages rather than on the actual types.

[gdey]

Hey, thanks for the first pass.

Some answers to your question before getting to the review:

1. The == in test file fails on comparing objects. Wondered what your thoughts were on making
   equals() methods for the types or if you had an idea on how to compare these other than ==? Or if for some reason I am using == wrong.

Are you trying to compare geom objects? There is an entire library that will allow you to compare geom objects cmp. The other way to compare complex object is to use reflect.DeepEqual; however that has some caveats.

2. Were either of you thinking of a different way to do this? I am trying to figure out ways to tokenize > for poly etc so i will expand the regex as I look at the types.

Yes, so, my plan was to use pegeon (or goyacc) and go generate to create the parser in an internal package to the wkt package. I would like to stay away from regular expressions so that we can be more control over errors, and additional tooling. It would, also, be nice to expose methods that expose the AST for additional tooling later.

Question 3 was answered by @ARolek; so I will skip.

I see the pro of having all this code in WKT, imports in 1 place. All the logic to encode the stuff is there as well. But if you had it as part of the interface you could just call it without the switches and just call gg.encode(). For decode you need the switch still to find the object but then you can just take the () and pass that to the object as needed to make itself.

We are keeping the geom package and the types simple.

---

A few general observations:

For Encode functions, we should follow the following form:

Encode(w io.Writer, geo geom.Geometry)error
EncodeBytes(geo geom.Geometry)([]bytes,error)
EncodeString(geo geom.Geometry)(string, error)

I know I'm not following this perfectly but this is the direction we want to be heading for all Geometry Encoders; there is still a discussion to be had on whether it should be EncodeBytes, and EncodeString or MarshalBytes and MarshalString.

---

For Decode functions, we should follow the following form:

Decode(r io.Reader) (geo geom.Geometry, err error)
DecodeBytes(b []byte) (geo geom.Geometry,err error)

Again there is still a discussion to be had on whether it should be DecodeBytes or UnmarshalBytes.

I don't have an interface set up yet as I want to see how this works for a few encoders before determining an interface for everyone to follow.

---

Minor change to the way we are doing tests (*still up for discussion as well).

Instead of doing:

fn := func(t *testing, tc tcase){ 
...
}
...
for name, tc := range tests {
   tc := tc
    t.Run(name, func(t *testing.T){ fn(t,tc)})
}

I'm starting to recommend we change to:

fn := func(tc tcase)func(t *testing){
     return func(t *testing){ 
...
     }
}
...
for name, tc := range tests {
    t.Run(name, fn(tc))
}

This eliminates the need to shadow tc; and makes it easier to read once you get passed the slightly awkward function sig.

[Hoovs]

Ty @ARolek and @gdey for getting back to me. @ARolek got it for 3, I will leave them as TODOs, ty for the info and it is very logical

@gdey I tried doing if !cmp.GeometryEqual(tc.Geom, grep) { in the unit test but the issue is for EMPTY. In the unit tests you see things like

"empty": {
                Geom: (*geom.Point)(nil),
                Rep:  "POINT EMPTY",
            },

Well that throws a NPE because it tries to get XY() of a nil. So the 2 fixes are to either add this to the check like

func PointerEqual(geo1, geo2 geom.Pointer) bool {return (geo1 == (*geom.Point)(nil) && geo2 == (*geom.Point)(nil)) || PointEqual(geo1.XY(), geo2.XY()) }

or pass back a fully nil object. The check looks ugly now and has to go in each type, or we pass back a raw nil and then it has no type which also seems wrong.

I can use a deepEqual but it feels like this nil checking should exist somewhere already and sometimes deepEqual can compare in the wrong order etc and cause false negatives; or that is what i remember seeing before.

Ah interesting, yeah I looked at this but then looking at more sources online it seemed people did a lot of RE (of which I am terrible at) so I thought they might be onto something. I agree it would be cleaner compiled as generators in internal, will look at that shortly.

Will take the observations into account as I keep tweaking the PR. Ty again for the answers. I made the PR but it is all regex for now.

[Hoovs]

I was looking at pigeon but I noticed that this repo seems to have no dependencies based on the root readme as well as just looking at the imports. Are you ok with adding it as a dependency to the project?

[gdey]

Yes, I'm okay adding dependency that are for tooling. Since pigeon would be a tool that is used to generate code that is then checked in. We don't need pigeon to compile or use this library (only to develop on the library). I would vendor pigeon that way we can continue to us the same version of pigeon to regenerate the parser as required.

[gdey]

Well that throws a NPE because it tries to get XY() of a nil. So the 2 fixes are to either add this to the check like

We should treat this as a bug, in the compare routine.

[Hoovs]

@gdey I have never used PEG but I gave it a super light crack.

I have been reading up on PEG on and off since your comment above.

If you're willing to give me a little feedback along the way I'd like to finish this but I am not sure I am going down the right track. Any advice would be amazing and no rush. Thank you :)

[gdey]

Hey @Hoovs,

Sorry, for disappearing, life... I'm going to take a look at this today and see, where you got to.

[Hoovs]

@gdey No worries, I know everyone is always busy; not enough hours in the day :)

I didn't check in the source code since I wasn't 100% sure on how you want to handle that. If it should be generated or source controlled hence the tests failing.

Please let me know if anything seems super off.

[ear7h]

Closing this bc #69 implemented wkt.Decode, contribution of a peg parser would still be welcome :)