CoordinatesType should extend Debug #597
Labels
Comments
Closed
frewsxcv
changed the title
Closed
|
I'm working on implementing this now for the upcoming geo-type 0.7 release |
bors bot
added a commit
that referenced
this issue
Jan 13, 2021
602: +Debug for CoordinateType r=frewsxcv a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [TODO] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- This is intended to address #597 I took a broader approach, and wonder if people are interested in it. The tldr is: ### geo-types - deprecate `CoordinateType`; (now includes `std::fmt::Debug`) - Add `trait CoordNum: CoordinateType;` - Add `trait CoordFloat: CoordNum + num_traits::Float` ### geo Add `trait GeoNum: CoordNum + HasKernel` Add `trait GeoFloat: CoordFloat + HasKernel` **downstream adoption** If your crate interops with geo-types, you may need to make changes like this: ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where - T: num_traits::Float, + T: geo_types::CoordFloat, { ``` or this (this one is actually only a deprecation, so you could leave it as is for now, but eventually you'll need to make the change): ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where - T: geo_types::CoordinateType + T: geo_types::CoordNum { ``` **alternative considered** If I went with the minimal approach to adding Debug trait, changes would instead need to look like this: This one, for non-floats, could remain untouched: ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where T: geo_types::CoordinateType { ``` But anything restricted to floats would need to explicitly include Debug ``` pub fn foo<T>(line_string: &geo_types::LineString<T>) where - T: num_traits::Float, + T: num_traits::Float + std::fmt::Debug, { ``` **motivation:** I initially completed #597 with the most conservative approach, achieved in the first two commits. The first simply adds Debug to CoordinateType and the second fixes the resulting compiler errors. This is a breaking change for anyone constraining generic implementations to just `num_traits::Float`, e.g. like [geojson crate does](https://github.com/georust/geojson/blob/master/src/conversion/to_geo_types.rs#L31) (fix here: https://github.com/georust/geojson/compare/mkirk/geo-types-0.7?expand=1), and polylabel (fix here: https://github.com/urschrei/polylabel-rs/compare/master...michaelkirk:mkirk/geo-types-0.7?expand=1) Compare this to a crate like [geo-booleanop](https://github.com/21re/rust-geo-booleanop/blob/master/lib/src/boolean/helper.rs#L61), for which things "just work" because they targeted CoordinateType. This got me thinking that, similar to #577 (yet unreleased), if we want people to write code targeting geo-types in a generic way, it might be helpful to make it clearer and easier how to do so. So, I've added a CoordFloat and a CoordNum which is basically just a rename of CoordinateType, and applied them liberally within our own code base. I'm hoping this would drive others to reach for these traits rather than specific num_traits when implementing code intended for generic geo-types interop. For crates like geo-booleanop, which targeted CoordinateType, things "just work" after the update, but they'll see a deprecation notice like: ``` $ cargo build warning: use of deprecated trait `geo_types::CoordinateType`: use `CoordFloat` or `CoordNum` instead --> lib/src/boolean/helper.rs:2:29 | 2 | use geo_types::{Coordinate, CoordinateType}; | ^^^^^^^^^^^^^^ | = note: `#[warn(deprecated)]` on by default ``` Which I think is acceptably clear. It's a large number of lines changed change overall, I know! But it's mostly mechanical, and I thought, since we're breaking geo-types, maybe this is a good time to bite the bullet and avoid some future pain. If it's too controversial, as I said, the minimal amount of change we need to enable Debug for CoordinateType is the first three commits (through and including "address fallout of "Add Debug to CoordinateType"). It's still a pretty big change with just that, and note that this will still break the crates I mentioned. I intend to fix the ones I know about, but there's a long tail of crates, which is why I hate breaking geo-types. In the few that I've checked it's been quick to adapt to the change, but it might be worth announcing and letting sit open for a while in case downstream geo-types users want to weigh in. 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
#596 (comment)
The text was updated successfully, but these errors were encountered: