-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] Expanded SF types to include MM types. #5
Conversation
Nice! Would be nice to have focused PRs though, perhaps all those TODOs are a bit much for all in this PR? |
Sure. I won't add more implementations/easy checks here. But the Types are there now, including a bounding box one. Let me know what you think. Only thing left is maybe add a method bounds that results in a BoundingBox with a fallback that computes it from all coordinates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the need for a concrete BoundingBox type, and I think this is quite nicely done.
Normally I'd favor using the GeometryBasics HyperRectangle
type for this, see this and that. But of course as an interface we cannot take on such a dependency (and we need the reverse dependency).
How do you see the extra complexity of the Line
, LinearRing
, Triangle
, Box
(and other) types? Just thinking aloud here. If I want to support Polygon
but not the special versions Triangle
and Box
, I guess an implementation can just cover AbstractPolygon
, and use it to create Polygon
types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update the readme where needed?
Since `geomtype` is expected to be defined for the instance, not the type itself.
After some thought I removed the bounding box type. It had some issues, such as the I've added the I've also started adding docstrings and did a split for types and interfaces, as requested by @rafaqz in #2. Some points that still need to be addressed:
Still some work to do, but let me know your thoughts on this direction. |
I'd say, barring obvious errors or typos, it's ready to merge. It isn't done yet, as the test suite needs expanding, especially for the default cases, but it's good enough for JuliaCon. |
is the error that this seems to give on CI on Julia 1.0
Resolves #2 and resolves #3.