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

Migrate Line/LineString to be a series of Coordinates (not Points). #244

Merged
merged 1 commit into from
Jun 23, 2018

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jun 3, 2018

Relevant conversation in #15.

This is a breaking change for geo-types.

I'm not planning to publish a release if this merges. I have a couple other breaking changes I'd like to consider before bumping the Rust geo ecosystem.

@frewsxcv frewsxcv force-pushed the frewsxcv-linestring-line-coords branch from e9a720b to 2099592 Compare June 3, 2018 15:26
@frewsxcv frewsxcv changed the title Migrate Line/LineString to be a series of Coordinates (not Points). Migrate Line/LineString to be a series of Coordinates (not Points). [WIP] Jun 4, 2018
frewsxcv added a commit that referenced this pull request Jun 19, 2018
frewsxcv added a commit that referenced this pull request Jun 20, 2018
frewsxcv added a commit that referenced this pull request Jun 20, 2018
bors bot added a commit that referenced this pull request Jun 20, 2018
278: Allow constructing a Line from anything that's Into<Coordinate> r=frewsxcv a=frewsxcv

Extracted froom #244.

280: Utilize `hypot` helper method. r=frewsxcv a=frewsxcv



Co-authored-by: Corey Farwell <[email protected]>
@frewsxcv frewsxcv force-pushed the frewsxcv-linestring-line-coords branch from d621246 to 4e7532a Compare June 23, 2018 03:39
@frewsxcv frewsxcv requested review from urschrei and jdroenner June 23, 2018 03:52
@frewsxcv
Copy link
Member Author

A pretty big pull request, but it should be ready to go now! I'll squash before approving

Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

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

This looks good to me, mod the few nits. Is it worth including a few lines on the distinctions between Coordinate and Point types from prior conversations (#15) in either the Point or Coordinate docstrings, just for context?

where
T: CoordinateType,
{
/// Returns the dot product of the two points:
Copy link
Member

Choose a reason for hiding this comment

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

should this be "…of the two coordinates"?

self.x * coord.x + self.y * coord.y
}

/// Returns the cross product of 3 points. A positive value implies
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -141,8 +141,27 @@ where
/// # );
/// ```
///
/// [determinant]: https://en.wikipedia.org/wiki/Determinant
Copy link
Member

Choose a reason for hiding this comment

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

FYI: links have to be inline for RLS

/// ```
///
/// You can iterate over the points in the `LineString`
/// You can iterate over the coordintes in the `LineString`
Copy link
Member

Choose a reason for hiding this comment

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

Typo.

@frewsxcv frewsxcv changed the title Migrate Line/LineString to be a series of Coordinates (not Points). [WIP] Migrate Line/LineString to be a series of Coordinates (not Points). Jun 23, 2018
@frewsxcv frewsxcv force-pushed the frewsxcv-linestring-line-coords branch 5 times, most recently from cfac035 to f312406 Compare June 23, 2018 14:30
@frewsxcv frewsxcv force-pushed the frewsxcv-linestring-line-coords branch from f312406 to b82b3a5 Compare June 23, 2018 14:39
@frewsxcv
Copy link
Member Author

bors r=urschrei

bors bot added a commit that referenced this pull request Jun 23, 2018
244: Migrate Line/LineString to be a series of Coordinates (not Points). r=urschrei a=frewsxcv

Relevant conversation in #15.

This is a breaking change for `geo-types`.

I'm _not_ planning to publish a release if this merges. I have a couple other breaking changes I'd like to consider before bumping the Rust geo ecosystem.

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

bors bot commented Jun 23, 2018

Build succeeded

@bors bors bot merged commit b82b3a5 into master Jun 23, 2018
@frewsxcv frewsxcv deleted the frewsxcv-linestring-line-coords branch June 26, 2018 02:56
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