Lab 7 - Errors #573
Merged
Lab 7 - Errors #573
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
kasunfdo
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe,
rokane and
samuong
as code owners
golangcibot
reviewed
Aug 9, 2019
kasunfdo
force-pushed
the
lab7
branch
2 times, most recently
from
7d37597 to
0f76be8
Compare
Codecov Report
@@ Coverage Diff @@
## master #573 +/- ##
======================================
Coverage 100% 100%
======================================
Files 191 156 -35
Lines 3611 2841 -770
======================================
- Hits 3611 2841 -770
Continue to review full report at Codecov.
|
juliaogris
suggested changes
Aug 12, 2019
07_errors/kasunfdo/leveldbstore.go
Outdated
| log.Printf("Initialized store with index: %v\n", store.count) | ||
| return store, nil | ||
| } | ||
| logrus.Debug(err) |
There was a problem hiding this comment.
Return on error instead?
There was a problem hiding this comment.
Hi @juliaogris ,
Any hint on testing error paths? I couldn't hit some of the error paths and that causes the build failure due to lack of code coverage.
07_errors/kasunfdo/leveldbstore.go
Outdated
| logrus.Debug(err) | ||
|
|
||
| value, err := db.Get([]byte(store.indexName), nil) | ||
| logrus.Debug(err) |
07_errors/kasunfdo/leveldbstore.go
Outdated
| logrus.Debug(err) | ||
|
|
||
| store.count, err = strconv.ParseUint(string(value), 10, 64) | ||
| log.Printf("Initialized store with index: %v", store.count) |
07_errors/kasunfdo/error.go
Outdated
| } | ||
|
|
||
| func NewError(code int, args ...interface{}) *Error { | ||
| return &Error{Code: code, Message: fmt.Sprintf(ErrStr[code], args...)} |
There was a problem hiding this comment.
IMO this should be something like the following (doesn't compile as Cause is only partially folded in)/.
type ErrCode uint8
const (
ErrInvalidInput ErrCode = 400
// ...
)
func (e ErrCode) String() string {
switch e {
case ErrInvalidInput:
return "invalid input"
// ....
}
}
NewError(code int, msg string, args ...interface{}) *Error {
return &Error{
Message: fmt.Sprintf(format, args...),
Code: code,
}
}
type Error struct {
Message string
Code ErrCode
Cause error // can be useful if wrapping underlying error
}
func (e *Error) Error() string {
msg := fmt.Sprintf("%s: %s", e.Code.String(), e.Message)
if e.Cause == nil {
return msg
}
return fmt.Sprintf("%s\n\t%s", msg, e.Cause.Error())
}
kasunfdo
force-pushed
the
lab7
branch
2 times, most recently
from
eb3db45 to
43bb19c
Compare
nicko-lee
reviewed
Aug 27, 2019
Codecov Report
@@ Coverage Diff @@
## master #573 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 204 208 +4
Lines 3865 3942 +77
=====================================
+ Hits 3865 3942 +77
Continue to review full report at Codecov.
|
kasunfdo
force-pushed
the
lab7
branch
3 times, most recently
from
9359085 to
9bb23cf
Compare
BaiJieCN
pushed a commit
to BaiJieCN/go-course
that referenced
this pull request
Sep 3, 2019
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.
Fixes #572
Review of colleague's PR #574
Changes proposed in this PR: