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.

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

docs update & bump to 0.12 #128

Merged
merged 5 commits into from
Nov 27, 2024
Merged

docs update & bump to 0.12 #128

merged 5 commits into from
Nov 27, 2024

Conversation

kylebarron
Copy link
Member

@kylebarron kylebarron commented Nov 26, 2024

  • 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

  • Update top-level crate documentation with new information about geo-traits
  • Add simple docstrings to all public functions
  • Remove panic

@kylebarron kylebarron changed the title docs update docs update & bump to 0.12 Nov 26, 2024
@michaelkirk
Copy link
Member

Should we make the switch to std::fmt::Write before releasing?

@kylebarron
Copy link
Member Author

kylebarron commented Nov 26, 2024

Should we make the switch to std::fmt::Write before releasing?

We already have std::fmt::Write. I was thinking more whether we should switch to std::io::Write. But it sounds like no?

@michaelkirk
Copy link
Member

michaelkirk commented Nov 26, 2024

Originally I meant "we should go all in on fmt::Write and change the existing ToWkt::write_wkt to accept a fmt::Write".

But now I'm starting to think the opposite and that maybe we should change all the new API's to use io::Write.

The main sticking point for me: It seems like while I can stream WKT to a File (and friends) with io::Write, I can't do that with fmt::Write. I have to first build up the entire string in memory with the Fmt API and then write that entire string in one go.

For big geometries, being able to buffer the writes out as you process seems like a better way to go.

WDYT @kylebarron ?

@kylebarron
Copy link
Member Author

The main sticking point for me: It seems like while I can stream WKT to a File (and friends) with io::Write, I can't do that with fmt::Write.

I think you can still stream WKT to io::Write with a small adapter: https://github.com/georust/wkt/pull/126/files#diff-afc819097d10bcb89581940115867e201017cd2bc9413c58f01b3dd610170f83R39-R49

Every call to write_str gets translated into the underlying io::Write::write, right? So I don't think there's any buffering still.

It's mostly a little ugly that you lost the content of any io::Error because that gets mapped to a fmt::Error.

@michaelkirk
Copy link
Member

I think you can still stream WKT to io::Write with a small adapter: https://github.com/georust/wkt/pull/126/files#diff-afc819097d10bcb89581940115867e201017cd2bc9413c58f01b3dd610170f83R39-R49

You're right again!

It's mostly a little ugly that you lost the content of any io::Error because that gets mapped to a fmt::Error.

I might see a way around this... after I wrap up some georust/geo stuff, I'll take a swing at 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.

LGTM

Anything else you want to include before releasing @kylebarron or anyone else?

Otherwise, I'll merge and release this tomorrow to give anybody else a final chance to weigh in.

@kylebarron
Copy link
Member Author

It looks good to me! #125 would be nice to get to in the medium term, but I don't think it should block a release.

@michaelkirk michaelkirk merged commit a8b8a1e into main Nov 27, 2024
1 check passed
@michaelkirk michaelkirk deleted the kyle/docs-update branch November 27, 2024 21:40
@michaelkirk
Copy link
Member

released: https://crates.io/crates/wkt/0.12.0
tagged: https://github.com/georust/wkt/releases/tag/0.12.0

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.

2 participants