-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: suggestion how to improve spverrors #749
Conversation
Manual Testsℹ️ Remember to ask team members to perform manual tests and to assign |
@@ -0,0 +1,157 @@ | |||
package spverrors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these changes now, shouldn't SPVError in models be removed? I dont see them being used anywhere now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I wanted to present the key concept for now.
If you liked it, I would try to adjust the whole codebase to it.
... Also removing SPVError
.
} | ||
return errors.Wrap(err, format+" caused by") | ||
return NewErrorf(format, args...).Wrap(err) | ||
} | ||
|
||
// Newf creates a new error with a message (which can be formatted) | ||
func Newf(message string, args ...interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this and the above method are calling exported methods and doesn't do anything else, wouldn't it be better to isntead of using Wrapf
to use NewErrorf
everywhere? I understand this was made to make less changes in existing code, but what do you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be better - and I will change it - but after
we agree that we want to go for it 😄
@@ -89,7 +89,7 @@ func (c *TaskManager) loadTaskQ(ctx context.Context) error { | |||
c.options.taskq.queue = q | |||
if factoryType == FactoryRedis { | |||
if err := q.Consumer().Start(ctx); err != nil { | |||
return spverrors.Wrapf(err, "failed to start consuming tasks from redis: %v") | |||
return spverrors.Wrapf(err, "failed to start consuming tasks from redis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no arguments are here I think it should be NewError("failed to start consuming tasks from redis").Wrap(err) but it's a minor, you can leave it as it is, just a nick pick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Will update in "actual" PR 👍
PublicMessage string | ||
HTTPStatus int | ||
|
||
code string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also any particular reason to make these fields private?
AI generated summary
This pull request introduces significant changes to the error handling mechanism across multiple files in the codebase. The primary focus is on replacing the existing error wrapping approach with a new custom error type,
spverrors.Error
, and updating related functions to use this new type.Error Handling Improvements:
New Custom Error Type:
engine/spverrors/error.go
: Introduced a newspverrors.Error
type to standardize error handling with additional context and HTTP status codes.engine/spverrors/error_test.go
: Added tests for the newspverrors.Error
type to ensure its functionality.Refactoring Error Wrapping:
actions/transactions/internal/mapping/outline/outline_request_to_engine.go
: Replacedspverrors.Wrapf
withspverrors.ErrCannotMapFromModel.Wrap
for more consistent error handling.engine/model_draft_transactions.go
: Updated error wrapping to usespverrors.Of(utxoErr).Wrap
instead ofspverrors.Wrapf
.engine/model_save.go
: Refactored error wrapping to usespverrors.Of(afterErr).Wrap
andspverrors.Of(err).Wrap(baseError)
. [1] [2]internal/query/parse.go
: Replacedspverrors.Wrapf
withspverrors.ErrCannotParseQueryParams.Wrap
for error handling.Utility and Test Updates:
Utility Functions:
engine/spverrors/utils.go
: UpdatedWrapf
andNewf
functions to utilize the newspverrors.Error
type and added a newOf
function to createspverrors.Error
from existing errors.Test Adjustments:
engine/spverrors/unfold_test.go
: Added test cases to validate the new error wrapping and handling mechanisms. [1] [2]These changes collectively enhance the robustness and consistency of error handling throughout the codebase.
Pull Request Checklist