solana icon indicating copy to clipboard operation
solana copied to clipboard

bank.rs too large for github diff view syntax highlighting

Open t-nelson opened this issue 2 years ago • 9 comments

Problem

$ wc -l runtime/src/bank.rs
20160 runtime/src/bank.rs

bank.rs, containing some of the most runtime-critical code in the repo, is too large for syntax highlighting in diff view on Github. this makes code review, especially of large change sets, tedious, if not error prone. inviting bugs like this is not The Way :tm:

Proposed Solution

we already have a module dir for bank, let's put it to better use!

  1. ~~move tests submodule to its own file~~ (https://github.com/solana-labs/solana/pull/29912)
  2. move ancillary types and impls to their own files
  3. encapsulate low-hanging fruit to own type/impls in their own files
  4. more risky stuff depending on where the above gets us

t-nelson avatar Dec 23 '22 19:12 t-nelson

is too large for syntax highlighting in diff view

And too large to load /blame 😢

I'm in favor of the proposed solution, even though we previously declined a contributor's Bank refactor as too dangerous.

CriesofCarrots avatar Dec 23 '22 20:12 CriesofCarrots

move tests submodule to its own file

This will get us 12k+(ish) (20k - 8k) out image

steviez avatar Dec 23 '22 22:12 steviez

Is the code coverage high enough to at least reduce the risk of such change?

denis-rafailov avatar Dec 30 '22 03:12 denis-rafailov

I think we can close this issue now that Bank.rs is well under the 20k syntax highlighting threshold. Any objections?

bw-solana avatar Feb 09 '23 18:02 bw-solana

I think we can close this issue now that Bank.rs is well under the 20k syntax highlighting threshold. Any objections?

I think it would still be nice to explore bullets 2 and 3 in the proposed solution.

CriesofCarrots avatar Feb 09 '23 19:02 CriesofCarrots

description updated

t-nelson avatar Feb 09 '23 19:02 t-nelson

I'd still like to see more progress here. Reopening.

CriesofCarrots avatar Feb 20 '24 17:02 CriesofCarrots

Ha, you beat me to it Tyera, but 100% agreed. bank.rs is more manageable, but we've somewhat just shifted the problem as /runtime/src/bank/tests.rs is now approaching 14k lines.

I'm not sure of the line-counts for each of these across source and tests, but I think two good candidates for their own file under /runtime/src/bank/ would be rewards and rent. I can take a pass at that in today or tomorrow

steviez avatar Feb 20 '24 17:02 steviez

in /runtime/src/bank/tests.rs file, there are 8 impls that should be moved to their structs. Also, 61 non-test functions can be defined in a separate file (e.g. /runtime/rc/bank/test_utils.rs). That will remove 2000-2500 LOC. Also, I suggest creating a folder runtime/src/bank/tests/, and separating tests into files there (or maybe into /runtime/tests/).

kubanemil avatar May 14 '24 17:05 kubanemil