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

Use coord! macro instead of Coordinate ctor #760

Merged
merged 5 commits into from
Mar 11, 2022
Merged

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Mar 8, 2022

This PR updates the geo-types crate. See also #761.

The coord! macro is much more flexible way to create a coordinate, and should be used in the same way as all other ones like line_string and polygon.

This change is a noop for now, but in the future it will make it easier to introduce Z coordinate without substantial code change.

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

The `coord!` macro is much more flexible way to create a coordinate, and should be used in the same way as all other ones like `line_string` and `polygon`.

This change is a noop for now, but in the future it will make it easier to introduce Z coordinate without substantial code change.
///
/// let point = Point(Coordinate { x: 1.5, y: 0.5 });
/// let dot = point.dot(Point(Coordinate { x: 2.0, y: 4.5 }));
/// let point = Point(coord! { x: 1.5, y: 0.5 });
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer

let point = Point::new(1.5, 0.5);

in doc comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

thx @pka will fix. BTW, you can use the suggestion button to actually propose a change - will make it much easier to submit the changes. Use this button in the comment window
image

Copy link
Member Author

Choose a reason for hiding this comment

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

@pka, I updated the PR, but TBH, I think this is wrong to introduce two unrelated changes in the same PR -- I was simply converting Coordinate {...} to coord! {...}, whereas this is technically a code change. A minor one, but these types of things we may probably want to do in a subsequent PRs

Copy link
Member Author

Choose a reason for hiding this comment

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

@pka seems like it would create more problems than solve -- https://github.com/georust/geo/runs/5472344274?check_suite_focus=true -- so reverting. Let's keep changes to a minimum per above, and consider other changes separately.

error[E0283]: type annotations needed for `geo_types::Point<{float}>`
  --> src/point.rs:237:13
   |
6  | let point = Point::new(1.5, 0.5);
   |     -----   ^^^^^^^^^^ cannot infer type for type `{float}`
   |     |
   |     consider giving `point` the explicit type `geo_types::Point<{float}>`, where the type parameter `{float}` is specified
   |

Copy link
Member

Choose a reason for hiding this comment

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

How about using the geo::point! { x: , y: } macro?

Copy link
Member

Choose a reason for hiding this comment

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

But yeah I'm fine keep it as you have written since you just did a find/replace, and this is unrelated

Copy link
Member Author

Choose a reason for hiding this comment

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

@frewsxcv I agree that we should refactor it further in a subsequent PR, flattening macro usage. Feel free to r plus it when ready :)

@nyurik nyurik mentioned this pull request Mar 10, 2022
2 tasks
@nyurik nyurik requested review from frewsxcv and michaelkirk March 10, 2022 21:18
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.

thank you!

@frewsxcv
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 11, 2022

Build succeeded:

@bors bors bot merged commit 6c2bc67 into georust:master Mar 11, 2022
@nyurik nyurik deleted the coord_macro branch March 11, 2022 16:11
bors bot added a commit that referenced this pull request Mar 11, 2022
761: Use coord! macro instead of Coordinate ctor (part 2) r=frewsxcv a=nyurik

This PR updates `geo` crate.  See also #760.

The `coord!` macro is much more flexible way to create a coordinate, and should be used in the same way as all other ones like `line_string` and `polygon`.

This change is a noop for now, but in the future it will make it easier to introduce Z coordinate without substantial code change.

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Yuri Astrakhan <[email protected]>
nyurik added a commit to nyurik/geo that referenced this pull request Mar 11, 2022
Accidentally missed coord! macro cleanups after the georust#760 and georust#761
@nyurik nyurik mentioned this pull request Mar 11, 2022
1 task
bors bot added a commit that referenced this pull request Mar 14, 2022
765: A few minor coord! followup cleanups r=frewsxcv a=nyurik

Accidentally missed coord! macro cleanups after the #760 and #761

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- ~[ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.~
---



Co-authored-by: Yuri Astrakhan <[email protected]>
nyurik added a commit to nyurik/geo that referenced this pull request Mar 16, 2022
Minor simplification of many `Point( coord! { ... })` into a more straightforward `point! { ... }` as initially proposed in prior PR georust#760 (review)
@nyurik nyurik mentioned this pull request Mar 16, 2022
1 task
bors bot added a commit that referenced this pull request Mar 16, 2022
773: Simplify Point(coord!()) into point! r=frewsxcv a=nyurik

Minor simplification of many `Point( coord! { ... })` into a more straightforward `point! { ... }` as initially proposed in prior PR #760 (review)

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- ~[ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.~
---



Co-authored-by: Yuri Astrakhan <[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.

3 participants