refactor: simplify large functions#2130
Closed
faddat wants to merge 265 commits intocometbft:mainfrom
Closed
Conversation
TestGenPrivKeySecp256k1
TestClientReplacesPrimaryWithWitnessIfPrimaryIsUnavailable
execAnsible function
This reverts commit ba108c6.
rpc/jsonrpc/client/args_test.go, mempool/clist_mempool_test.go, crypto/merkle/proof_test.go, and internal/store/store_test.go
and types/block_test.go
cache_test.go, clist_mempool_test.go, helpers_test.go, and status_test.go
This was referenced Jan 31, 2024
Collaborator
|
I can see that you've already started breaking this PR into smaller PRs. I think this is the right direction 👍 |
Contributor
Author
|
If it's no different to you, I would like to reopen this as a draft pull request for tracking purposes. This PR enables several of the linters that you mentioned in #2274 . Would you like to go linter by linter or how do you want to do it? |
35 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR will enable some new linters, and be based off of:
main + #1907
... so #1907 should be merged before this one, because that will greatly reduce the number of changed files from 203 to somewhere in the teens. If at any time it's needed to break any of this into its own PR's, just let me know.
New Linters
I got (in my opinion) amazing results from this. After the configuration, I was guided to what are more or less known trouble areas in comet, nearly every time.
Ongoing plan
Over time, we can lower the complexity rules and nestif to their defaults, and I think that this would likely have a great effect on the software overall.
Notes
I really liked some of the changes I started to get as I worked with these linters, which are all related to each other to some degree.
This PR takes some of the largest functions in cometbft, and breaks them into smaller pieces.
When I started it, I wasn't too sure, but by using this approach I was definitely able to make changes that seem to:
Changes unique to this PR
This PR takes anything that the nestif/cyclomaticcomplexity/cognitivecomplexity linters finds to be too complex, and refactors it into individual functions. I wasn't too sure about this course of action when I started, but in a number of places, the results I started to get are (I think) quite convincing.
In places where the actual code has been changed, I'm finding that the reworked style is actually easier to read and think about.
It's likely that I will write out the whole PR, kind of like
And then submit the whole thing as a series of smaller patches.
For example, this technique has led me to poolRoutine, which is indeed a large and complex function. It runs consistently, and it is probably better for many reasons to break it into separate functions.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments