georust / wkt

Rust read/write support for well-known text (WKT)
https://crates.io/crates/wkt
Apache License 2.0
51 stars 25 forks source link

Prepare for 0.10.0 release #77

Closed urschrei closed 2 years ago

urschrei commented 2 years ago
nyurik commented 2 years ago

@urschrei could you merge the two pending PRs first if they are ok?

urschrei commented 2 years ago

Holding off on this and will wait until https://github.com/georust/wkt/pull/75 and https://github.com/georust/wkt/pull/76 merge, all else being equal

michaelkirk commented 2 years ago

Since this has "breaking" changes, shouldn't this be 0.10.0 ?

urschrei commented 2 years ago

Since this has "breaking" changes, shouldn't this be 0.10.0 ?

Ohhh. Yes.

nyurik commented 2 years ago

If we are doing a breaking change, there is something lippy suggests that would require it. Note that this is the last clippy warning that kinda should be fixed. And we can enable CI linting (fmt + clippy) as we did in geo

warning: method `from_str` can be confused for the standard trait method `std::str::FromStr::from_str`
   --> src/lib.rs:190:5
    |
190 | /     pub fn from_str(wkt_str: &str) -> Result<Self, &'static str> {
191 | |         let tokens = Tokens::from_str(wkt_str);
192 | |         Wkt::from_tokens(tokens)
193 | |     }
    | |_____^
    |
    = note: `#[warn(clippy::should_implement_trait)]` on by default
    = help: consider implementing the trait `std::str::FromStr` or choosing a less ambiguous method name
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait
urschrei commented 2 years ago

@nyurik In that case how about a PR that deprecates (but doesn't remove) from_str in favour of e.g. from_wkt_str? I don't want to make that change unilaterally.

lnicola commented 2 years ago

This is a small breaking change in comparison to some of the recent PRs (mainly #72), I don't think it's worth adding a deprecation.

urschrei commented 2 years ago

This is a small breaking change in comparison to some of the recent PRs (mainly #72), I don't think it's worth adding a deprecation.

We've never just removed a function like this, though. I know that in practice it's not likely to affect anyone outside georust, but it feels wrong to just remove it.

nyurik commented 2 years ago

sec, we are not removing it, will submit a PR in a sec

urschrei commented 2 years ago

sec, we are not removing it, will submit a PR in a sec

Already done in 74a454d!

nyurik commented 2 years ago

@urschrei i don't think that's a good approach - mine makes it far less intrusive, just adding a use statement fixes it for the users (hence the "breaking" change). See #79

urschrei commented 2 years ago

@urschrei i don't think that's a good approach - mine makes it far less intrusive, just adding a use statement fixes it for the users (hence the "breaking" change). See #79

Yep, pulled it from this PR.

nyurik commented 2 years ago

you really like your force pushes, eh? :) I usually prefer to keep merging things into the PR until its ready, and use Squash and Merge to keep history clean. This way any comments left on the code in github gets tracked properly (and don't get lost by accident). Plus it usually makes for far fewer conflict resolutions during the development. But that's just me :)

nyurik commented 2 years ago

Found a bug related to #79 -- fixed in #80 and added to CI so that it wouldn't be missed next time

urschrei commented 2 years ago

Yes I am a huge fan of various flavours of rebase and force-push. One day, I will regret it. But not today.

urschrei commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded: