Skip to content
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

Generalise geomtypes #305

Merged
merged 3 commits into from
Jul 4, 2022
Merged

Generalise geomtypes #305

merged 3 commits into from
Jul 4, 2022

Conversation

rafaqz
Copy link
Collaborator

@rafaqz rafaqz commented Jun 8, 2022

This PR allows creating points with any Real numbers.

It also cleans up the code generation code used for createpoint etc, simplifying and reducing indentation level.

Closes #304

@yeesian
Copy link
Owner

yeesian commented Jun 12, 2022

Thank you for this! It seems the only remaining method to be covered is createmultipolygon(::Vector{Vector{T} where T}). Apart from that, the rest seem to be pertaining more to some of the GeoInterface methods to be implemented.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jun 14, 2022

Sorry where is the missing createmultipolygon? I can't see where that was even originally implemented.

Edit: sorry I see it's just missing types on in the tests.

Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

The implementation looks much cleaner, and the drop in test coverage is a weakness of the tests rather than the implementation so this LGTM, thank you!

I appreciate some of the additional tests you've added! Additional increases in test coverage can happen in the future is isn't a blocker here from my POV.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jul 4, 2022

Yes sometimes when you delete code you delete more lines that were covered than lines that weren't, so it looks like you've reduced test coverage. But the amount of funcitonality covered hasn't actually changed.

@rafaqz rafaqz merged commit 04d067e into yeesian:master Jul 4, 2022
@rafaqz rafaqz deleted the generalise_geomtypes branch July 4, 2022 01:47
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.

Can't create geometry with non-Float64 vectors
2 participants