search

geocaml / ocaml-geojson

A library for manipulating, creating and parsing GeoJSON
https://geocaml.github.io/ocaml-geojson
Other
38 stars 10 forks source link

Add tests for 3d #33

Closed streetCoderr closed 2 years ago

streetCoderr commented 2 years ago

We have tested for Geometry, Feature and FeatureCollection; we have also tested for when they include bbox; but in all these, we have only taken 2d into consideration. Knowing that a position can be represented in 3 dimensions, lets add tests to account for 3d.

streetCoderr commented 2 years ago

Hi @patricoferris, I would like to work on this. Can you assign this issue to me. Thank you as I await your response

patricoferris commented 2 years ago

Sounds like a great idea to me, thanks :))

streetCoderr commented 2 years ago

Sir I would love to hear your thoughts on this issue.

I was thinking 2 tests would suffice for this.

Test 1

My initial thought was that since we have written 2d tests for the GeoJson types, we are already confident that all is working as expected. What we are not sure of yet, is if it would still behave as expected if we are dealing with 3d positions. So I was thinking we can pick up just one of the GeoJSON types, and test for 3d and that would cover up for the rest. For instance, because FeatureCollection embodies Feature, and Feature in turn, embodies Geometry. We could just test for 3d on only FeatureCollection and then draw conclusion for Feature and Geometry from there.

Test 2

For the second test, we test reading bbox from 3d provided positions on any chosen GeoJSON type.

What do you think? Is this okay?

Or should I test for 3d on all GeoJSON types?

Thank you, as I await your response.

streetCoderr commented 2 years ago

Hi @patricoferris, I think I would rather test on all the GeoJSON types. I don't want to assume anything about the expected behaviour. It would be better we leave no stone unturned here.

patricoferris commented 2 years ago

Hey, sorry for the slow response.

I think I would rather test on all the GeoJSON types.

I don't think we need to do that. The coordinate system, whether 2D or 3D, is confined to the Point/Position geometry type which the others are based off. I think going for "Test 1" for now will be sufficient and we can incrementally add more if we find holes in the tests:))

Thanks for the diligent work ^^

streetCoderr commented 2 years ago

Hey, sorry for the slow response.

I think I would rather test on all the GeoJSON types.

I don't think we need to do that. The coordinate system, whether 2D or 3D, is confined to the Point/Position geometry type which the others are based off. I think going for "Test 1" for now will be sufficient and we can incrementally add more if we find holes in the tests:))

Thanks for the diligent work ^^

I thought as much. I was just not sure. Thank you for clearing my doubts. I will proceed with a PR now😊😊

Thanks for the diligent work ^^

I appreciate this. Thanks a lot

patricoferris commented 2 years ago

Fixed in #36. Thanks!