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

clippy: ledger-tool lints #34640

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Jan 3, 2024

Problem

There are new nightly clippy lints in the ledger-tool source. See the parent issue #34626 for more context

Summary of Changes

Apply the change suggested below

warning: `flatten()` will run forever if the iterator repeatedly produces an `Err`
    --> ledger-tool/src/main.rs:2649:39
     |
2649 |                 for line in f.lines().flatten() {
     |                                       ^^^^^^^^^ help: replace with: `map_while(Result::ok)`
     |
note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error
    --> ledger-tool/src/main.rs:2649:29
     |
2649 |                 for line in f.lines().flatten() {
     |                             ^^^^^^^^^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok
     = note: `#[warn(clippy::lines_filter_map_ok)]` on by default

warning: `solana-ledger-tool` (bin "solana-ledger-tool" test) generated 1 warning

warning: `flatten()` will run forever if the iterator repeatedly produces an `Err`
    --> ledger-tool/src/main.rs:2649:39
     |
2649 |                 for line in f.lines().flatten() {
     |                                       ^^^^^^^^^ help: replace with: `map_while(Result::ok)`
     |
note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error
    --> ledger-tool/src/main.rs:2649:29
     |
2649 |                 for line in f.lines().flatten() {
     |                             ^^^^^^^^^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok
     = note: `#[warn(clippy::lines_filter_map_ok)]` on by default

warning: `solana-ledger-tool` (bin "solana-ledger-tool" test) generated 1 warning
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you confirm this still works as intended? (I wasn't familiar with the lines() stuff, which is why I didn't do this one myself heh.)

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d6146af) 81.8% compared to head (212937e) 81.8%.
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34640   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         824      824           
  Lines      222262   222257    -5     
=======================================
+ Hits       181860   181920   +60     
+ Misses      40402    40337   -65     

@steviez
Copy link
Contributor Author

steviez commented Jan 3, 2024

Looks good to me. Can you confirm this still works as intended? (I wasn't familiar with the lines() stuff, which is why I didn't do this one myself heh.)

Can confirm - lines() returns an iterator that yields Result<String, Error>. Calling .flatten() on that yields the Ok(String)s and skips the Err(...)s.

On the other hand, calling map_while() directly yields us all Ok(String)s until we hit an Err(...). So, technically a slight change in behavior in that the old method would continue past an error (potentially infinitely per the lint) while the new approach will halt at first error. An error would be something like the read bytes not being valid UTF-8 or some low level error with the file; I think bailing on either of these events is just fine.

@steviez steviez merged commit 1d93732 into solana-labs:master Jan 3, 2024
35 checks passed
@steviez steviez deleted the lt_new_rust_clippy_lints branch January 3, 2024 19:35
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.

2 participants