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.

Sign up for GitHub

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

Jump to bottom

Use generic type for precision in conversion #43

Merged
merged 2 commits into from Oct 26, 2019
Merged

Use generic type for precision in conversion #43

merged 2 commits into from Oct 26, 2019

Conversation

pjsier
Copy link
Member

@pjsier pjsier commented Oct 6, 2019

Attempt to close #33. I based this off of the conversion implementation in geojson and only used generic types for converting to geo_types objects

Initially I tried to update the From trait implementation, but I used Into instead because updating From gave me the following error:

type parameter `T` must be used as the type parameter for some local type

If I missed something that would make From work instead I can make that change. Let me know if I should change that or anything else here, thanks!

@frewsxcv
Copy link
Member

frewsxcv commented Oct 6, 2019

The code changes you made look good! One thing I had in mind for #33 (which I didn't articulate well) is for the f64 types in Coord to also utilize num_traits::Float. So when WKT parsing happens, the numeric values get parsed into the generic type. Does that kinda make sense?

@pjsier
Copy link
Member Author

pjsier commented Oct 6, 2019

Makes sense, thanks! Wasn't sure how comprehensive you were thinking the changes should be, but that works.

It looks like some Coord functionality would require trait implementations on the generic type for fmt::Display and str::FromStr (and potentially others). Should I just include those traits in the definitions where they're needed and default to using num_traits::Float, or should there be a type defined at the root that implements all the necessary traits in addition to num_traits::Float like CoordinateType does in geo-types?

@frewsxcv
Copy link
Member

frewsxcv commented Oct 6, 2019

I'm fine with either of your options! Probably not going to be much of a difference for users of this crate

@pjsier
Copy link
Member Author

pjsier commented Oct 6, 2019

Gotcha, I just made those changes. I ended up needing to use std::marker::PhantomData to give the Tokens struct ownership of the generic type, but I'm not sure if that was the best option here

@pjsier
Copy link
Member Author

pjsier commented Oct 20, 2019

@frewsxcv let me know if there’s anything I should fix here when you have a chance, thanks!

@frewsxcv
Copy link
Member

@frewsxcv let me know if there’s anything I should fix here when you have a chance, thanks!

Nope, looks excellent! Thanks so much!

@frewsxcv
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 26, 2019
@bors @pjsier
Merge #43
1faf2b3 
43: Use generic type for precision in conversion r=frewsxcv a=pjsier

Attempt to close #33. I based this off of the conversion implementation in [geojson](https://github.com/georust/geojson/blob/master/src/conversion.rs) and only used generic types for converting to `geo_types` objects

Initially I tried to update the `From` trait implementation, but I used `Into` instead because updating `From` gave me the following error:

```
type parameter `T` must be used as the type parameter for some local type
```

If I missed something that would make `From` work instead I can make that change. Let me know if I should change that or anything else here, thanks!

Co-authored-by: pjsier <pjsier@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 26, 2019

Build succeeded

@bors bors bot merged commit 5aa1fc4 into georust:master Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frewsxcv frewsxcv frewsxcv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use generic type for precision
2 participants
@pjsier @frewsxcv