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

refactor: address clippy warnings + code cleanup #125

Merged
merged 7 commits into from
Feb 5, 2024
Merged

refactor: address clippy warnings + code cleanup #125

merged 7 commits into from
Feb 5, 2024

Conversation

null8626
Copy link
Contributor

@null8626 null8626 commented Feb 4, 2024

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:

  • Using matches! over manual match or if let statements.
  • Simplify while-loop and for-loop definitions.
  • Using .find(...) over .filter(...).next().
  • Using .flatten() for some nested for-loops.
  • Remove DirectionalStatusStack as it's only used in one function in the codebase, and basically a wrapper over a Vec<Status> that doesn't do anything different.

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

@mbrubeck
Copy link
Contributor

mbrubeck commented Feb 4, 2024

Compatibility notes:

  • matches! requires Rust 1.42.0 (released March 2020).
  • Range::is_empty requires Rust 1.47.0 (released October 2020).

@Manishearth
Copy link
Member

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
Copy link
Member

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.

#126 to mark it in Cargo.toml (this will make clippy less noisy about features that don't exist on MSRV)

@mbrubeck
Copy link
Contributor

mbrubeck commented Feb 4, 2024

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 #126 (comment).

@Manishearth
Copy link
Member

I'm fine with moving to 1.47

@null8626
Copy link
Contributor Author

null8626 commented Feb 5, 2024

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

@Manishearth
Copy link
Member

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

@null8626
Copy link
Contributor Author

null8626 commented Feb 5, 2024

Alright

@null8626
Copy link
Contributor Author

null8626 commented Feb 5, 2024

Done

@Manishearth Manishearth merged commit b3fc605 into servo:master Feb 5, 2024
7 checks passed
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