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

Fix several strategies (add getters, fix compilation error and warning) #1364

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

awulkiew
Copy link
Member

The getters are missing for centroid and closest_points in index and global umbrella strategies preventing them to be passed to these algorithms and rtree.

I divided centroid into detail part and official part derived from base. It works for now because centroid doesn't have proper strategies for spherical and geographic. After they are added and e.g. geographic one uses Formula and Spheroid we'll probably have to implement this differently.

@awulkiew
Copy link
Member Author

I added one more fix in geo distance_cross_track_box_box strategy.

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

Thanks!

@vissarion
Copy link
Member

I do not know if it makes sense to switch to commit messages as proposed in #1324, now we have a mix of commit messages.

@awulkiew
Copy link
Member Author

I missed that and used the old pattern. I altered the commit messages.

It would probably be useful to have it written down in the gudelines. I can see there are some differences between your messages and the article as well, e.g. doc vs docs, ext vs extension, starting from small vs big letter, etc. Also kinds/categories of changes are mixed with library parts like fix/feat vs extension. Fix/feat should probably be used for extensions as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants