Skip to content

Conversation

ear7h
Copy link
Contributor

@ear7h ear7h commented Dec 1, 2019

closes #42

@ear7h ear7h requested review from gdey and ARolek December 1, 2019 04:04
@coveralls
Copy link

coveralls commented Dec 1, 2019

Pull Request Test Coverage Report for Build 455

  • 17 of 137 (12.41%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 54.788%

Changes Missing Coverage Covered Lines Changed/Added Lines %
utils.go 17 137 12.41%
Totals Coverage Status
Change from base Build 443: -0.6%
Covered Lines: 5836
Relevant Lines: 10652

💛 - Coveralls

@gdey
Copy link
Member

gdey commented Dec 1, 2019

Okay, looking through this commit, it highlights something things I did not think about. I think Clone makes sense to add, (I think if we are going to have this method called CloneGeo, we should spell it out to CloneGeometry.)

Right now we depend on the fact that geom.Geometry is an interface{}, this is really useful for the wkt/wkb encode methods. and the Decomposition of our current types. What I mean by this is:
geom.Multipolygon's Polygons() returns [][][2]float64 values, these no longer qualify as geom.Geometry. Making printing out or dealing with parts of Multi or even regular Geometries harder.

I know you changed the type of Encode to a plain interface{} but this does not really tell the user of the Encode function what it can take. The primary purpose of geom.Geometry was to document that this function takes things that are well-known geom geometry types. Adding a function to the interface now allows the Compiler to enforce this, but the way we have our geometries structured (returned base types e.g. [2]float64, [][2]float64, etc...) goes against this.

Because of this, I think we need to not have this as part of geom.Geometry, rather we should spin up a new Interface that only has this method.

We then should discuss if it makes sense to keep geom.Interfaces that way they are or should they return geom types.

What I mean is should: Linestring.Vertices(), return []geom.Point; instead of [][2]float64.

This is, of course, a reversal back to what we originally had. The main reason for switching to the basic types was the hassle in Go of having to recast from [2]float64 to geom.Point... However, we recent releases of Go
this is less of a pain, but still a pain.

If we keep the base go types, then I don't think it makes sense to have any functions in geom.Geometry. As that will make working with those functions harder.

Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

utils.go Outdated
}
}

// CloneGeomtry returns a deep clone of the Geometry.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Clone

@gdey gdey merged commit 8a0ff4e into master Dec 3, 2019
@gdey gdey deleted the issue-42 branch December 3, 2019 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add members to the geometry interface
3 participants