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

Add conditional Eq impl for Coordinate, maybe other types? #431

Merged
merged 1 commit into from
Mar 29, 2020

Conversation

nick-parker
Copy link
Contributor

@nick-parker nick-parker commented Mar 23, 2020

I haven't written tests or anything for this but I wanted to throw the PR in to get discussion started.

Many geometry algorithms eg this optimal algorithm for computing mesh cross-sections and reconstructing the polygons use Hashtables or similar to keep track of coordinates. This impl lets me put geo Coordinate structs in the Hashtable instead of dealing with a separate struct just for this step.

I haven't put much thought into whether similar one-liner Eq impl's should be added for Linestring, Polygon, etc but I don't really see why not. The use cases are rarer but probably still exist.

Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me! Yeah +1 to adding the same impl for all the other core geo types

@frewsxcv
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Mar 29, 2020
431: Add conditional Eq impl for Coordinate, maybe other types? r=frewsxcv a=nick-parker

I haven't written tests or anything for this but I wanted to throw the PR in to get discussion started.

Many geometry algorithms eg this [optimal algorithm for computing mesh cross-sections and reconstructing the polygons](https://www.researchgate.net/publication/309619647_An_Optimal_Algorithm_for_3D_Triangle_Mesh_Slicing) use Hashtables or similar to keep track of coordinates. This impl lets me put geo Coordinate structs in the Hashtable instead of dealing with a separate struct just for this step.

I haven't put much thought into whether similar one-liner Eq impl's should be added for Linestring, Polygon, etc but I don't really see why not. The use cases are rarer but probably still exist.

432: Add Triangle and Rect to Geometry. r=frewsxcv a=frewsxcv

Fixes #390

Extracted from https://github.com/georust/geo/pull/421/files

Co-authored-by: Nick Parker <[email protected]>
Co-authored-by: Corey Farwell <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 29, 2020

This PR was included in a batch that timed out, it will be automatically retried

@bors
Copy link
Contributor

bors bot commented Mar 29, 2020

Build succeeded

@bors bors bot merged commit 2763279 into georust:master Mar 29, 2020
bors bot added a commit that referenced this pull request Apr 2, 2020
435: derive Eq for other core geo types r=frewsxcv a=michaelkirk

Adds `Eq` conformance to other core geo types, building off of #431

> Seems reasonable to me! Yeah +1 to adding the same impl for all the other core geo types

Rather than spelling out the impl, I've taken advantage of how `derive` already bounds the generated impl for us. See rust-lang/rust#21237 for more details

I wrote a few doc-tests while developing, but I reverted them in fc33f9f since they're pretty verbose, and seem superfluous since they're only asserting rust language semantics rather than anything substantial in this lib.

I can un-revert them if you'd prefer. 

Co-authored-by: Michael Kirk <[email protected]>
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.

2 participants