Add common error prefix #291
Conversation
| @@ -391,3 +392,13 @@ func TestSetNamespace(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func compareErrors(err1, err2 error) bool { | |||
There was a problem hiding this comment.
I think reflect.DeepEqual does exactly what compareErrors does.
There was a problem hiding this comment.
The underlying stack trace captures will be different:
There was a problem hiding this comment.
Damn. That's not nice since the error itself doesn't change. It doesn't really matter where it occurs.
There was a problem hiding this comment.
I've added a comment to this function referring to your play.golang.org link. My guess is that this will bite us at some point when users expect reflect.DeepEqual to work.
There was a problem hiding this comment.
Just got bitten by this in two ways. First, that errors now adds a prefix and then that the comparison did not work. I'll expose the compare function so that we can use that in tests but it would be nice if we could find a more generic solution.
| @@ -3,7 +3,7 @@ package uapolicy | |||
| import ( | |||
| "crypto" | |||
| "crypto/hmac" | |||
| "errors" | |||
| "github.com/gopcua/opcua/errors" | |||
| "sort" | ||
|
|
||
| "github.com/gopcua/opcua/errors" | ||
|
|
There was a problem hiding this comment.
are you using a different fmt tool? go fmt doesn't report any issues this or uapolicy/crypto_hmac.go
There was a problem hiding this comment.
No, I'm just a stickler for these things. I believe in consistency and that developers don't read code but pattern match it. So any deviation from the structure slows you down b/c you fall back to the slower "read code" mode.
| @@ -124,7 +124,7 @@ func (s *SecureChannel) SendRequest(req ua.Request, authToken *ua.NodeID, h func | |||
| func (s *SecureChannel) SendRequestWithTimeout(req ua.Request, authToken *ua.NodeID, timeout time.Duration, h func(interface{}) error) error { | |||
There was a problem hiding this comment.
this belongs to a different patch
There was a problem hiding this comment.
I have no idea how this is contaminating my branch. I even redid this PR from scratch.
See #291 (comment) for discussion
Fixes #253