Consolidate error types into a single error type #12
Conversation
jsternberg
force-pushed
the
consolidate-error-types
branch
2 times, most recently
from
29a646d to
f5b96b5
Compare
errors.go
Outdated
| "errors" | ||
| import "context" | ||
|
|
||
| type code int |
There was a problem hiding this comment.
This could use an int8.
This type could also just be the base error type without wrapping. Then keep the error struct only when adding a custom message.
There was a problem hiding this comment.
I restored the customMessage type and the wrapping code related to that and changed this type to an int8 and renamed this type to Error. This will still have the consolidated type but it should be more simple.
jsternberg
force-pushed
the
consolidate-error-types
branch
from
f5b96b5 to
2682980
Compare
This consolidates the various error types to a single error type with a code that designates the type of error. This reduces the amount and complexity of the overall code. The individual methods like `IsInvalidArgument` that support both errors from this package and moby errors are now moved to the `compat.go` file to organize them together. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
jsternberg
force-pushed
the
consolidate-error-types
branch
from
2682980 to
acf19c1
Compare
| } | ||
|
|
||
| func (c customMessage) Error() string { | ||
| return c.msg | ||
| func (m customMessage) Unwrap() error { |
There was a problem hiding this comment.
Why Unwrap vs using the Is and As or even keeping both?
There was a problem hiding this comment.
I honestly thought I was reverting this and thought this was the original code.
I do think Unwrap() is probably better since it causes the standard library to do things properly and makes it so there's fewer things to test. While it isn't in this PR, I think customMessage is likely not needed as its own type and could be replaced by Attach or Mask from #16, but Attach and Mask provide a pattern that's more composable in other situations.
For example, say I want to annotate an existing error with an error from errdefs. I would either have to make another error type that wraps errdefs or I could utilize one of the functions in that PR and get the same behavior.
| switch e := err.(type) { | ||
| case customMessage: | ||
| err = e.err | ||
| case Error: |
There was a problem hiding this comment.
Should keep the custom error case rather than rely on Unwrap
There was a problem hiding this comment.
I suspect that keeping customMessage here when the more traditional Unwrap() works might be an anti-pattern and creates the possibility of more dead code.
Is there a reason why it would be better to keep customMessage here rather than rely on Unwrap()?
| ) | ||
|
|
||
| type Error int8 |
There was a problem hiding this comment.
Can you rename this to errNum or ErrNum if a const must be an exported type.
|
Please split out the changes to |
|
@jsternberg Could you update the PR? |
|
We don't want a single error type, see #18 for explanation. If we have a major version bump of this package, the errors implementing the defined interface will guarantee that at least the error types remain consistent. |
This consolidates the various error types to a single error type with a code that designates the type of error. This reduces the amount and complexity of the overall code.
The individual methods like
IsInvalidArgumentthat support both errors from this package and moby errors are now moved to thecompat.gofile to organize them together.