Skip to content

Conversation

@kylebarron
Copy link
Member

@kylebarron kylebarron commented May 8, 2025

  • 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.
  • I ran cargo fmt

cc @Kontinuation

@kylebarron kylebarron requested a review from michaelkirk May 8, 2025 17:06
Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

We just had a breaking release 2 weeks ago. Maybe we should cool it for a second. Is there anything else breaking on the horizon?

@kylebarron
Copy link
Member Author

kylebarron commented May 8, 2025

I agree it's unfortunate.

Speaking for myself, I'd really like to get a release of geoarrow on crates.io (the last one was like 9 months ago). The core geoarrow crates depend on geo-traits, wkb, and wkt (though not geo or geo-types). geo-traits is not as widely used as geo-types obviously but at least for me I feel some painful churn when we publish a new version.

If I publish a new geoarrow version with geo-traits 0.2, then I at least need a new wkb release because I need georust/wkb#53.

So for me the core question is whether we want to release a new wkb version with geo-traits 0.2 and geo-traits 0.3 or whether we should include geo-traits 0.3 in the next wkb version (which would necessitate a wkt release with geo-traits 0.3 as well)

@michaelkirk
Copy link
Member

If geo-traits is going to have breaking releases more frequently than its client libraries (e.g. wkt), maybe an approach like we take with the geo-types+rstar integration would be helpful to avoid disruptive churn across the ecosystem.

Traveling back in time: basically, leave the old geo-traits (v0.2) integration in place in wkt, so that we don't break anything, and then add a new geo-traits-0_3 feature with the new geo-traits-v0.3 integration.

And then when we eventually do have a breaking release in wkt, you can also update the primary geo-traits integration, and remove the version gated feature flags.

@kylebarron
Copy link
Member Author

If geo-traits is going to have breaking releases more frequently than its client libraries (e.g. wkt)

Well hopefully not; I want to think it's stable, but it's still young 😄

Traveling back in time: basically, leave the old geo-traits (v0.2) integration in place in wkt, so that we don't break anything, and then add a new geo-traits-0_3 feature with the new geo-traits-v0.3 integration.

I was looking at the geo-types Cargo.toml and there rstar is an optional dependency, so the feature flag turns on different versions. But here geo-traits is a non-optional dependency. How do we switch between different geo-traits versions from feature flags for a required dep?

@michaelkirk
Copy link
Member

But here geo-traits is a non-optional dependency. How do we switch between different geo-traits versions from feature flags for a required dep?

I'd guess we wouldn't switch, but rather that we'd have potentially more than one integration.

Also, is there a reason that geo-traits is required, rather than optional?

@kylebarron
Copy link
Member Author

I'd guess we wouldn't switch, but rather that we'd have potentially more than one integration.

Can you give a concrete example of how this would work? I tried to explore this in #149 , but if the dependencies are non-optional I'm not sure how this works.

Also, is there a reason that geo-traits is required, rather than optional?

It could be optional on the read side, but it's not-optional on the write side, where we always use write APIs that are based on geo-traits:

wkt/src/to_wkt/mod.rs

Lines 94 to 98 in dc0af7f

fn write_wkt(&self, writer: impl io::Write) -> io::Result<()> {
let mut writer_wrapper = WriterWrapper::new(writer);
write_geometry(&mut writer_wrapper, &self.to_wkt())
.map_err(|err| writer_wrapper.into_io_err(err))
}

@michaelkirk
Copy link
Member

Also, is there a reason that geo-traits is required, rather than optional?

It could be optional on the read side, but it's not-optional on the write side, where we always use write APIs that are based on geo-traits:

Ah! I forgot that the inner write logic had a hard dependency on geo-traits, so it seems like making it optional is not possible. That's fine, I just didn't remember why.

Can you give a concrete example of how this would work?

Here's a start:
https://github.com/georust/wkt/tree/mkirk/non-breaking-geo-traits-update

It doesn't currently compile because I haven't implemented it for every geometry variant (which is required for the base GeometryTrait implementation), but I think it should work.

If we were to go that route, I think I'd first consolidate the geo_trait impl's into a single module, rather than having them scattered across the code base.

Unfortunately, I don't have more time to spend on this. If you want to do the optional implementation, that'd be nice. Out of practicality, we could also just consider this a learning experience, and try again next time we're about to commit a breaking change right after a release.

@kylebarron
Copy link
Member Author

kylebarron commented May 12, 2025

Out of practicality, we could also just consider this a learning experience, and try again next time we're about to commit a breaking change right after a release.

Sometimes it's hard to know when other breaking changes will come along. It's not clear if there was any way to know when we were making the wkt 0.13 release that we'd have another geo-traits release soon.

Ah! I forgot that the inner write logic had a hard dependency on geo-traits, so it seems like making it optional is not possible. That's fine, I just didn't remember why.

Given that the writing is non-optional, is it worth making the read side optional?

Here's a start: mkirk/non-breaking-geo-traits-update

Ok, so the main change there is that geo-traits v0.2 is left as the name geo_traits, while geo-traits v0.3 is given the name geo_traits_0_3. But that branch doesn't seem to touch writing at all? It's still not clear with that branch how to handle writing. Would we export a second write module with geo-traits v0.3?

@michaelkirk
Copy link
Member

Would we export a second write module with geo-traits v0.3?

I think so. I think to avoid breaking v0.2, you'd need the v0.3 integration to be name spaced somewhere else so people could opt into it.

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

I'm OK to release this and have it be another breaking release, though I'd prefer that in the future we try harder before merging breaking changes so close to a previous breaking release.

@kylebarron kylebarron merged commit 7330c45 into main May 14, 2025
1 check passed
@kylebarron kylebarron deleted the kyle/prepare-0.14 branch May 14, 2025 15:34
@kylebarron
Copy link
Member Author

Agreed, we should try harder to coalesce breaking changes in the future. Hopefully this will be a one-off.

@kylebarron
Copy link
Member Author

Ugh I'm dumb 😭

version = "0.13.0"

Another PR incoming

@kylebarron kylebarron mentioned this pull request May 14, 2025
3 tasks
@michaelkirk
Copy link
Member

michaelkirk commented May 14, 2025

Coalescing is good. Strategies to achieve it are:

  1. being prescient 😉
  2. sitting on breaking changes for a while

Another strategy is to put more effort into making the changes opt in and non-breaking.

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