Skip to content

Migrate to edition 2021#967

Open
Philippe-Cholet wants to merge 5 commits intorust-itertools:masterfrom
Philippe-Cholet:edition-2021
Open

Migrate to edition 2021#967
Philippe-Cholet wants to merge 5 commits intorust-itertools:masterfrom
Philippe-Cholet:edition-2021

Conversation

@Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Jul 4, 2024

I noted 5 months ago that edition 2021 requires rustc 1.56+ so it's now possible.
Is there any reason to not update?

@codecov
Copy link

codecov bot commented Jul 4, 2024 •
edited
Loading

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.65%. Comparing base (6814180) to head (fde1ad9).
⚠️ Report is 168 commits behind head on master.

Files with missing lines Patch % Lines
src/adaptors/mod.rs 91.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
- Coverage   94.38%   93.65%   -0.74%     
==========================================
  Files          48       50       +2     
  Lines        6665     6323     -342     
==========================================
- Hits         6291     5922     -369     
- Misses        374      401      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Philippe-Cholet Philippe-Cholet marked this pull request as draft July 4, 2024 10:50
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Jul 4, 2024 •
edited
Loading

I added some marginal changes:

  • MSRV automated in the crate documentation (valid for a MSRV >= 1.63.0).
  • Fix some additional clippy lints, mostly related to the recent MSRV/edition changes.
  • Use ControlFlow?!

@jswrenn
I remember your #780 (comment) where you prefered ControlFlow over Result, I replaced some, left some aside for now.
Can you elaborate on the reason?

@Philippe-Cholet Philippe-Cholet marked this pull request as ready for review July 4, 2024 13:14
@Philippe-Cholet Philippe-Cholet requested a review from jswrenn July 4, 2024 13:18
@jswrenn
Copy link
Member

jswrenn commented Aug 2, 2024

I remember your #780 (comment) where you prefered ControlFlow over Result, I replaced some, left some aside for now.
Can you elaborate on the reason?

Result::Err denotes "something went wrong"; ControlFlow::Break doesn't. Favor ControlFlow when we're trying to encode conditional iteration without judgement about error-ness, and favor Result when you're specifically dealing with an early return due to error.

jswrenn
jswrenn approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me; I'll let you hit 'Merge' when you're ready to do so.

src/lib.rs
//!
//! This version of itertools requires Rust 1.63.0 or later.
//! This version of itertools requires Rust
#![doc = env!("CARGO_PKG_RUST_VERSION")]
Copy link
Member

@jswrenn jswrenn Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge fan of this!

@jswrenn
Copy link
Member

jswrenn commented Oct 22, 2024

@Philippe-Cholet is this good to merge?

@Natural-selection1
Copy link

Natural-selection1 commented Nov 12, 2025

haha, it's been nearly ten months since the 2024 Edition was released. Maybe we can migrate a bit more

@Philippe-Cholet @phimuemue
Migrate to edition 2021
05f0db3 
Edition 2021 requires rustc 1.56+ so it's possible.
1. `cargo fix --edition`
2. Field "edition" updated in "Cargo.toml".
@Philippe-Cholet @phimuemue
Automate MSRV change in the crate doc
971ec97 
I noted some months ago that the environment variable CARGO_PKG_RUST_VERSION was publicized in 1.63.0 and that if we do not go back before 1.63 then we can automate the MSRV there.
Remain "ci.yml" but I don't see a way to automate it, even with `cargo metadata --no-deps --format-version=1 | jq -r '.packages[0].version'`.
@Philippe-Cholet @phimuemue
Use bool::then_some
93f4d39
@Philippe-Cholet @phimuemue
Simplifications from clippy
90a5afe 
`clippy::uninlined_format_args` and 2 more.
@Philippe-Cholet @phimuemue
ControlFlow instead of Result
fde1ad9
phimuemue
phimuemue reviewed Nov 13, 2025
View reviewed changes
Copy link
Member

@phimuemue phimuemue left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Philippe-Cholet

@jswrenn I think we should merge this. It improves the code quite a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phimuemue phimuemue phimuemue left review comments

@jswrenn jswrenn jswrenn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Philippe-Cholet @jswrenn @Natural-selection1 @phimuemue