refactor!: Remove panics #500
Conversation
|
This pull request introduces 2 alerts when merging adbf40d into dfde9a5 - view on LGTM.com new alerts:
|
|
Thanks for working on this! |
|
This pull request introduces 2 alerts when merging 2886aa5 into dfde9a5 - view on LGTM.com new alerts:
|
|
From the starting 49 panics we are now at 25. So 12 panics to go 🥳 I think I should be able to finish this up tomorrow, but I expect a lot of back and forth. I assume we might encounter cases in which we might go back to panics, but I can't tell. |
|
This pull request introduces 1 alert when merging c95063a into dfde9a5 - view on LGTM.com new alerts:
|
|
I'm in a full support for reducing panics. However, this will create lot of breaking changes in Cosmos SDK. So this has to be well coordinated. |
|
I'll put this as R4R, I'm missing the panics in KeyFormat but maybe it's better to do those in a different PR. This is getting too big and the KeyFormat methods seem to be everywhere |
| fmt.Printf("Hash: %X\n", tree.Hash()) | ||
| hash, err := tree.Hash() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error hashing tree: %s\n", err) |
There was a problem hiding this comment.
why not using log package?
There was a problem hiding this comment.
I think that's because it's a command that prints stuff to the command line output. So it uses fmt.Print everywhere
node.go
Outdated
| if node.isLeaf() { | ||
| panic("Attempt to copy a leaf node") | ||
| return nil, fmt.Errorf("attempt to copy a leaf node") |
There was a problem hiding this comment.
let's create const errors and return them.
eg:
const ErrCloneLeafNode = fmt.Errorf("attempt to copy a leaf node")
There was a problem hiding this comment.
Added them everywhere I replaced a panic (and the error is not dynamic). More work will be needed on this later on
|
this looks amazing!! |
WIP, removing some panics by returning errors instead. Looking to solve #212