-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add wrapper types for all geometries #78
Conversation
@evetion if you want to have a look at this it would be good to get it merged soon so we can use it in tests. There are still a few types left to test but comments on overall structure would be good at this stage. The idea is geometry constructors and methods are all defined in an |
This is ready to go |
My first thought would be to not export them as they will often clash with other packages. Putting them in a module works, perhaps combined with making them available unexported in the GeoInterface namespace as well, such that Can you elaborate a bit on how/where/why you want to use these wrapper types? Is it still mainly to make it easier to work with Base types that we consider to be GeoInterface compatible objects, and arrays thereof, as mentioned in the first post? Maybe the answer could go straight to the docs ;) |
Reasoning:
|
Thanks! Would you mind adding this to the docs as well? Perhaps also including when it's not needed/helpful to use these? For packages like GeoJSON and GeometryBasics I guess there is not directly much benefit from these, or not? |
Yeah, I will add that. Its actually pretty beneficial to GeometryBasics.jl as it has no Feature/FeatureCollection concept, again see this for a use case #79. Also most packages (e.g. Rasters.jl) don't know about these packages or depend on them, so would still use the wrapper type over the package type as its generic. Even if GeoJSON has its own types how would we know about them without a dependency, and why would we write anything package specific when we don't have to. I imagine it will be the same for users. Just learn one set of wrappers and use them everywhere, even if there are native types why bother learning that. (this generality is made more useful with the PRs I'm making to ArchGDAL/LibGEOS etc - they don't care about geometry types anymore anything will work) |
ea9487f
to
c428863
Compare
This is good to go, with a section in the docs. I simplified it to just include the main types needed for e.g. LibGEOS tests. We can work through issues (like with how to construct |
🎉 I'll try to review tomorrow |
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.
Gargantuan effort, very meta!
Closes #75 |
@evetion thanks for the super-thorough review, gives me a lot of confidence in quality of the PR and the interface generally. It made me think we should aim for 100% coverage and this degree of correctness now that we are not rushing to just get the basics working. |
Ok this has 100% coverage. @evetion want to give this one last review? then we can squash and merge. (it really needs squashing, there are a lot of trash commits here) |
This PR adds wrapper types for all geometries.
The idea is these will work with wrapping
PointTrait
objects forLineString
orMultiPoint
.Basically you should be able to hack together any geometries you want using any package components and they will work.
It's not quite there yet.