search

georust / geozero

Zero-Copy reading and writing of geospatial data.
Apache License 2.0
352 stars 37 forks source link

Make other wkb and wkt variants generic #188

Closed kylebarron closed 8 months ago

kylebarron commented 8 months ago

This is a follow up to https://github.com/georust/geozero/pull/171 to make other WKB and WKT variants generic over any AsRef<[u8]> input

With this change, I think WktString and WktStr are redundant, but I didn't want to make a breaking change. I think this should be non-breaking because String and &str can AsRef to [u8] right?

kylebarron commented 8 months ago

Is the failing CI still a change in this PR? It seems unrelated

michaelkirk commented 8 months ago

Is the failing CI still a change in this PR? It seems unrelated

I think this will be fixed here https://github.com/georust/geozero/pull/189

kylebarron commented 8 months ago

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?

michaelkirk commented 8 months ago

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 #[allow(deprecated)] on our own internal usage.

michaelkirk commented 8 months ago

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).

michaelkirk commented 8 months ago

I say we merge this

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.

kylebarron commented 8 months ago

Do you want to keep the deprecated structs instead of removing them in this release?

michaelkirk commented 8 months ago

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.

kylebarron commented 8 months ago

Awesome, any other comments on this PR?

ariesdevil commented 8 months ago

When do we plan to release the new version on crates.io?