Return Errors instead of Panicing #30
Merged
Conversation
This file contains 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
- closes #28 - closes #4 Return errors and have the application decide how they want to handle panicing :smile Signed-off-by: Marko Baricevic <[email protected]>
tac0turtle
added
T:enhancement
tac0turtle
changed the title
golangcibot
reviewed
Nov 13, 2019
tac0turtle
changed the title
tac0turtle
commented
Nov 15, 2019
tac0turtle
commented
Nov 15, 2019
alessio
reviewed
Nov 15, 2019
alexanderbez
suggested changes
Nov 15, 2019
melekes
reviewed
Nov 21, 2019
- add comment about why the error is ignored - wraped a couple of errors with error.Wrap
tac0turtle
force-pushed
the
marko/ErrorNoPanic
branch
from
17b62f1 to
c72ac02
Compare
tac0turtle
commented
Dec 5, 2019
alexanderbez
approved these changes
Dec 6, 2019
| @@ -20,6 +20,8 @@ func TestBoltDBNewBoltDB(t *testing.T) { | |||
| db.Close() | |||
| } | |||
|
|
|||
| //TODO: ADD TESTS!!! | |||
There was a problem hiding this comment.
yes, there is an issue for it. To keep the scope of this pr small I will handle it in subsequent PRs,
|
LGTM - Great Job! |
melekes
reviewed
Dec 12, 2019
| func (itr *boltDBIterator) Next() { | ||
| itr.assertIsValid() | ||
| func (itr *boltDBIterator) Next() error { | ||
| if err := itr.assertIsValid(); err != nil { |
There was a problem hiding this comment.
assert should panic I believe
| itr.assertIsValid() | ||
| return itr.source.Key() | ||
| func (itr cLevelDBIterator) Key() ([]byte, error) { | ||
| if err := itr.assertNoError(); err != nil { |
There was a problem hiding this comment.
same. assertX should panic
| // but this is being conveyed in the below if statement | ||
| key, _ := source.Key() //nolint:errcheck | ||
|
|
||
| if !source.Valid() || !bytes.HasPrefix(key, prefix) { |
There was a problem hiding this comment.
the better way may be
pitr := &prefixIterator{
prefix: prefix,
start: start,
end: end,
source: source,
valid: false,
}
if !source.Valid() {
return pitr
}
key, err := source.Key()
if err != nil {
return nil, err
}
if !bytes.HasPrefix(key, prefix) {
return pitr
}| @@ -329,7 +383,7 @@ func stripPrefix(key []byte, prefix []byte) (stripped []byte) { | |||
| panic("should not happen") | |||
| } | |||
| if !bytes.Equal(key[:len(prefix)], prefix) { | |||
| panic("should not happne") | |||
| panic("should not happn") | |||
| if !itr.valid { | ||
| panic("prefixIterator invalid, cannot call Key()") |
There was a problem hiding this comment.
panic is appropriate here as it checks program logic (ie. nobody should call Key/Value if iterator is not Valid)
faddat
pushed a commit
to faddat/tm-db
that referenced
this pull request
Feb 21, 2024
…endermint#30) Bumps [golangci/golangci-lint-action](https://github.com/golangci/golangci-lint-action) from 3.3.1 to 3.4.0. - [Release notes](https://github.com/golangci/golangci-lint-action/releases) - [Commits](golangci/golangci-lint-action@v3.3.1...v3.4.0) --- updated-dependencies: - dependency-name: golangci/golangci-lint-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Return errors and have the application decide how they want to handle panicing :smile
Signed-off-by: Marko Baricevic [email protected]