You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
There are a number of errorx functions that rely on Cast implementation. Cast was designed in pre Go 1.13 times and it doesn't respect wrap semantic of Go 1.13. While I'm not sure whether Cast should respect it, but I'm pretty sure that functions like HasTrait, Ignore, IgnoreWithTrait, ExtractProperty and probably TraitSwitch and TypeSwitch should.
There are more places using Cast:
(ErrorBuilder).WithCause(error), (ErrorBuilder).EnhanceStackTrace() and (ErrorBuilder).assembleStackTrace(). Should it know about wrap? Probably yes.
(*Error).Is(target error). Should we accept that target could be a wrapped *errorx.Error. I am personally not sure here. Standard library implementations don't unwrap target which is an argument to stick to current behaviour.
(*Error).Property(Property). Should it work when underlying property is buried under non-errorx wrapper? Looks like it should. But I then see that Property being a method is a wrong abstraction. Should we hide the method in favor to ExtractProperty?
errorx.GetTypeName(error). I'm not sure what would be the least surprising behaviour but the following example definitely looks broken: https://play.golang.org/p/_UAGbNO2ZlH
There also is a errorx.WithPayload which accepts *errorx.Error as an argument. To call this function the client code have to cast error to *errorx.Error with something like errorx.Cast. Should we change errorx.WithPayload to accept error?
Also there are a lot of Cast usages in our private codebase that will benefit from Cast being wrap-aware.
Whooa, that was a lot of concerns. Should I split them to separate issues? Maybe, but let's discuss them first.
The text was updated successfully, but these errors were encountered:
Let's.
As for Cast itself, I believe we should split it: make a private cast work just as it works now, as other parts of the implementation rely on it, and make a separate decision about a public Cast. Maybe there even have to be several versions of each of those methods.
Should we remove a public Cast altogether? It's a breaking change, but are there legal usages right now? If there are, any proposal to change its behaviour must first dive deep into those cases, and make a case (if you pardon the pun) about how they benefit from a changed behaviour or at least do not get broken.
As for HasTrait, Ignore, IgnoreWithTrait, ExtractProperty etc., as soon as we agree that they can be based on some private versions of Cast, a PR can be made to change them, with tests to showcase the change.
There are a number of
errorx
functions that rely onCast
implementation.Cast
was designed in pre Go 1.13 times and it doesn't respect wrap semantic of Go 1.13. While I'm not sure whetherCast
should respect it, but I'm pretty sure that functions likeHasTrait
,Ignore
,IgnoreWithTrait
,ExtractProperty
and probablyTraitSwitch
andTypeSwitch
should.There are more places using
Cast
:(ErrorBuilder).WithCause(error)
,(ErrorBuilder).EnhanceStackTrace()
and(ErrorBuilder).assembleStackTrace()
. Should it know about wrap? Probably yes.(*Error).Is(target error)
. Should we accept thattarget
could be a wrapped*errorx.Error
. I am personally not sure here. Standard library implementations don't unwraptarget
which is an argument to stick to current behaviour.(*Error).Property(Property)
. Should it work when underlying property is buried under non-errorx wrapper? Looks like it should. But I then see thatProperty
being a method is a wrong abstraction. Should we hide the method in favor toExtractProperty
?errorx.GetTypeName(error)
. I'm not sure what would be the least surprising behaviour but the following example definitely looks broken: https://play.golang.org/p/_UAGbNO2ZlHThere also is a
errorx.WithPayload
which accepts*errorx.Error
as an argument. To call this function the client code have to casterror
to*errorx.Error
with something likeerrorx.Cast
. Should we changeerrorx.WithPayload
to accepterror
?Also there are a lot of
Cast
usages in our private codebase that will benefit fromCast
being wrap-aware.Whooa, that was a lot of concerns. Should I split them to separate issues? Maybe, but let's discuss them first.
The text was updated successfully, but these errors were encountered: