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

ledger-tool: Run rustfmt with format_strings = true #34284

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Nov 30, 2023

This PR spun out from #34274 per some conversation to consider running the rustfmt on a subset to see if it makes things more manageable.

Problem

Long string literals can cause rustfmt to fail, which results in rustfmt failing to format entire functions. There are several instances of this in ledger-tool, so format these files with wrapped strings so that formatting will apply to functions again.

Summary of Changes

Note that this PR was created by adding format_strings = true to rustfmt.toml; however, this change does NOT persist that rule as the rule would format the entire repo.

While reviewing the diff, I do see some lines that were wrapped that could be candidates for inlining the values in format strings:

error!("confirmed_block_exists() failed from the destination Bigtable, slot: {}, err: {}", slot, err);

However, there are similar instances in lines that are not wrapped, so I'm inclined to defer that

@steviez
Copy link
Contributor Author

steviez commented Nov 30, 2023

In the other PR, Tyera mentioned that strings like this could get mangled:

first half of the string\
second half of the string

Namely, this would get turned into

first half of the stringsecond of the string

whereas we most likely want

first half of the string second half of the string

due to the lack of a whitespace before the trailing \. Scanning this PR, I didn't see any instances except for the ones she called out, which I fixed in the second commit.

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #34284 (d7420f9) into master (18309ba) will decrease coverage by 0.1%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head d7420f9 differs from pull request most recent head fed7df1. Consider uploading reports for the commit fed7df1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34284     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      219969   219969             
=========================================
- Hits       180326   180297     -29     
- Misses      39643    39672     +29     

@steviez steviez marked this pull request as ready for review November 30, 2023 20:09
Long string literals can cause rustfmt to fail, which results in rustfmt
failing to format entire functions. There are several instances of this
in ledger-tool, so format these files with wrapped strings so that
formatting will apply to functions again.

Note that this PR was created by adding format_strings = true to
rustfmt.toml; however, this change does NOT persist that rule as the
rule would format the entire repo.
CriesofCarrots
CriesofCarrots previously approved these changes Dec 1, 2023
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

I suggested a couple default reformats to match what I did in solana-cli, but I don't feel strongly about them here. Feel free to merge as-is if you prefer.

ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@steviez
Copy link
Contributor Author

steviez commented Dec 1, 2023

I suggested a couple default reformats to match what I did in solana-cli, but I don't feel strongly about them here. Feel free to merge as-is if you prefer.

No, these are good ones that I missed on my pass through (think my eyes were starting to bleed from doing solana-cli and this one back-to-back haha).

@steviez steviez merged commit 7c4e723 into solana-labs:master Dec 1, 2023
33 checks passed
@steviez steviez deleted the lt_rustfmt branch December 1, 2023 08:50
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