Make other wkb and wkt variants generic #188
Conversation
|
Is the failing CI still a change in this PR? It seems unrelated |
I think this will be fixed here #189 |
| @@ -26,38 +26,38 @@ impl<B: AsRef<[u8]>> GeozeroGeometry for Wkb<B> { | |||
| )] | |||
| #[cfg_attr(feature = "with-postgis-diesel", diesel(sql_type = Geometry))] | |||
| #[cfg_attr(feature = "with-postgis-diesel", diesel(sql_type = Geography))] | |||
| pub struct Ewkb(pub Vec<u8>); | |||
There was a problem hiding this comment.
What do you think about a default trait implementation to make this change non-breaking (same for GpkgWkb, SpatiaLiteWkb, and MySQLWkb)?
There was a problem hiding this comment.
Oh I didn't realize that this was breaking, but I suppose it's breaking because the internal type is pub? So it still accepts input with Vec<u8> but when you call Ewkb(...).0 you no longer know you're getting a Vec<u8>?
There was a problem hiding this comment.
default trait implementation
Do you have an example of what you mean?
There was a problem hiding this comment.
Like this: pub struct Ewkb<B: AsRef<[u8]> = Vec<u8>>(pub B);
There was a problem hiding this comment.
error: defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions
--> geozero/src/wkb/wkb_reader.rs:29:17
|
29 | pub struct Ewkb<B: AsRef<[u8]> = Vec<u8>>(pub B);
| ^^^^^^^^^^^^^^^^^^^^^^^^
did I do that wrong?
There was a problem hiding this comment.
Hm. No, you did it right, but it looks like a bug in diesel's derive macros:
Here's an annotated snippet from cargo expand:
impl<'expr, B: AsRef<[u8]> = Vec<u8>> AsExpression<Geometry> for &'expr Ewkb<B> {
^^^^^^^^^^- THIS IS A PROBLEM!!!
type Expression = Bound<Geometry, Self>;
fn as_expression(self) -> Self::Expression {
Bound::new(self)
}
}
I'll file a bug report with the diesel people. I guess we should just eat the breaking change?
There was a problem hiding this comment.
Filed: diesel-rs/diesel#3902
There was a problem hiding this comment.
Fixed in diesel-rs/diesel@2eaf4aa. So fast!
geozero/src/wkt/wkt_reader.rs
Outdated
| @@ -7,34 +7,34 @@ use wkt::Geometry; | |||
|
|
|||
| /// WKT String. | |||
| #[derive(Debug)] | |||
| pub struct WktString(pub String); | |||
| pub struct WktString<B: AsRef<[u8]>>(pub B); | |||
There was a problem hiding this comment.
I agree that it's nice to be able to take either a String or a &str, but I think it's confusing to explain why we have two different functionally equivalent types like this.
What would you think about unifying them? Either by deprecating one or (my preference) revert and deprecate both legacy types and introduce a third unified struct (maybe called Wkt?).
(same for Ewkt)
There was a problem hiding this comment.
I agree that having multiple redundant types is confusing; I wasn't sure which would be preferred, but I'm happy to deprecate these and move to Wkt
I agree that it's nice to be able to take either a String or a &str
It's also nice that it can take in a Vec<u8> and &[u8] at the same time! There were a couple places that previously had copies with to_vec that could be removed
|
I haven't worked with deprecated objects in rust before and I don't know the right way to get clippy happy with deprecated but not yet removed objects. Thoughts? |
I would slap some |
|
Rather than wait for diesel to make a release, I say we merge this and accept that it's a small breaking change to our diesel integration (we're already looking at a semver breaking release as it is). |
Sorry to be sloppy with my language, I mean to say that we should revert the default trait implementation that I earlier suggested, since it's breaking the current release of diesel. And then merge this PR. |
|
Do you want to keep the deprecated structs instead of removing them in this release? |
Even though this is already going to be a breaking release, I'd prefer to keep them. As a user, I know I like having deprecations that tell me exactly what needs to change. |
|
Awesome, any other comments on this PR? |
|
When do we plan to release the new version on crates.io? |
This is a follow up to #171 to make other WKB and WKT variants generic over any
AsRef<[u8]>inputWith this change, I think
WktStringandWktStrare redundant, but I didn't want to make a breaking change. I think this should be non-breaking becauseStringand&strcanAsRefto[u8]right?