georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
410 stars 68 forks source link

Upgrade to heapless v0.7.10 and update MSRV to 1.51.0 #87

Closed rye closed 2 years ago

rye commented 2 years ago

Rust 1.51 has been released for nearly 12 months now.

As the community upgrades through and past this Rust version, some required dependencies of rstar (namely, heapless v0.6, which itself required the as-slice and generic-array dependencies before Rust 1.51) have since released updates to make use of the new Const Generics MVP functionality that was released with Rust 1.51.

To remove a few outdated dependencies and upgrade the codebase to the latest version of heapless, this PR does the following:

rmanoka commented 2 years ago

Just a minor nit: pls annotate the chanelog entry, mentioning this as a breaking change. Will bump the version appropriately when publishing the next release.

Looks good to me otherwise.

michaelkirk commented 2 years ago

Just a minor nit: pls annotate the chanelog entry, mentioning this as a breaking change.

Historically, there's been a lot of discussion on wether MSRV bump should be considered a breaking change. e.g. https://users.rust-lang.org/t/rust-version-requirement-change-as-semver-breaking-or-not/20980/39

I don't really understand how the rust-version constraint added to Cargo.toml since 1.56 will affect this, but since we're still supporting toolchains older than that, I suspect it's yet irrelevant.

rmanoka commented 2 years ago

I'm sorry if I wasn't clear. Upgrading heapless is what I considered breaking, not the msrv change.

rmanoka commented 2 years ago

I see I may be mistaken here. Is the heapless change visible in public API? If not it's not really breaking change, and we don't need to mention in the changelog.

Regarding, msrv, yeah it's debatable whether to call it a breaking change. @rye sorry for the confusion, but could you revert back to your previous changelog message?

rye commented 2 years ago

@rmanoka no problem, I have reverted back to the original state.

To be clear:

rmanoka commented 2 years ago

I'll give it a day if @michaelkirk or others have any more suggestions, and merge it otherwise. Thanks @rye !

michaelkirk commented 2 years ago

Thanks @rye!

bors r+

rmanoka commented 2 years ago

may be try more politely? please: bors r+

bors[bot] commented 2 years ago

Build succeeded: