servo / unicode-bidi

Implementation of the Unicode Bidirection Algorithm in Rust
Other
74 stars 34 forks source link

refactor: address clippy warnings + code cleanup #125

Closed null8626 closed 5 months ago

null8626 commented 5 months ago

Hi! Thank you so much for this package, probably one of the most reliable Unicode bidi libraries I've ever discovered.

The following pull request cleans up the codebase by addressing (most) clippy warnings while also cleaning up other things i've noitced too. Like:

If you have any feedback regarding this pull request, feel free to tell me! Cheers!

mbrubeck commented 5 months ago

Compatibility notes:

Manishearth commented 5 months ago

Yeah, I'm reluctant to accept PRs that bump MSRV unless we really need to.

(If you plan on updating this PR please add new commits undoing the changes that need MSRV so it's easy to review)

Manishearth commented 5 months ago

Yeah I'm reluctant to change MSRV just for code cleanliness. This crate is at the bottom of a bunch of long deptrees so we're conservative about MSRV.

https://github.com/servo/unicode-bidi/pull/126 to mark it in Cargo.toml (this will make clippy less noisy about features that don't exist on MSRV)

mbrubeck commented 5 months ago

Our major downstream crates (idna, stringprep, url) already require Rust 1.51 or later. The most important, url, requires Rust 1.56.

While I agree we need to be conservative with MSRV, I’m not sure we need to support toolchains that are more than 3 years out of date. Anyone using a 4-year-old compiler will generally need to keep their dependencies locked to old versions too.

So I personally would be comfortable bumping to Rust 1.47 even without a strong need. Conveniently, I believe Rust 1.47 is also the first version that can read v3 lockfiles, as you suggested in https://github.com/servo/unicode-bidi/pull/126#issuecomment-1925906481.

Manishearth commented 5 months ago

I'm fine with moving to 1.47

null8626 commented 5 months ago

So is it okay to merge the changes as-is?

Manishearth commented 5 months ago

You'd need to add a commit changing MSRV and MSRV CI to 1.47

null8626 commented 5 months ago

Alright

null8626 commented 5 months ago

Done