feat: Return sane errors from Translate #178
Conversation
sdboyer
changed the title
|
Still need to add some tests, here. fyi @mildwonkey |
errors/errors.go
Outdated
| // Translation errors. These all occur as a result of an invalid lens. Currently | ||
| // these may be returned from [thema.Instance.TranslateErr]. Eventually, it is | ||
| // hoped that they will be caught statically in [thema.BindLineage] and cannot | ||
| // occur at runtime. |
There was a problem hiding this comment.
Is that aspiration (no translation errors, just statically caught ones at bind time) fair as soon as we foresee the need of adding support for "delegated" translations? Or would that be another, separated, API? 🤔
NB: By "delegated" translations, I mean a translation that cannot be performed just with plain CUE but relies on external systems (like a DB) and/or requires another way to define it (in Go, for instance).
There was a problem hiding this comment.
From a user perspective, there's two conceivable approaches to delegating (i'll ponder that word, too):
- Inject go functions when calling
BindLineagethat completely take over lens responsibility, unconditionally - Inject go functions when calling
BindLineagethat are only called in the event of a matching lacuna emission from the CUE-specified lens
i suspect that these are close enough that we can make them the same thing, which would be lovely from a "keep thema's Go interface stable" perspective.
However, this comment is now a bit out of date in light of the change in thinking that we're just always going to allow lens delegation to go funcs - making that degraded mode a normal part of the contract means that a non-erroring Translate() is just permanently off the table.
There was a problem hiding this comment.
which would be lovely from a "keep thema's Go interface stable" perspective.
Perhaps adding functional options? I'd say it's quite idiomatic (fair?) and would leave us more flexibility.
making that degraded mode a normal part of the contract means that a non-erroring Translate() is just permanently off the table
Right, yeah! 😅
But I'm curious, how do you think that'd change with lacunas?
Would it be something like: errors vs "unsatisfied/unhandled" lacunas?
| var ( | ||
| // ErrInvalidLens indicates that a lens is not correctly written. It is the parent | ||
| // to all other lens and translation errors, and is a child of ErrInvalidLineage. | ||
| ErrInvalidLens = errors.New("lens is invalid") | ||
|
|
||
| // ErrLensIncomplete indicates that translating some valid data through | ||
| // a lens produced a non-concrete result. This always indicates a problem with the | ||
| // lens as it is written, and as such is a child of ErrInvalidLens. | ||
| ErrLensIncomplete = errors.New("result of lens translation is not concrete") | ||
|
|
||
| // ErrLensResultIsInvalidData indicates that translating some valid data through a | ||
| // lens produced a result that was not an instance of the target schema. This | ||
| // always indicates a problem with the lens as it is written, and as such is a | ||
| // child of ErrInvalidLens. | ||
| ErrLensResultIsInvalidData = errors.New("result of lens translation is not valid for target schema") | ||
| ) |
There was a problem hiding this comment.
When docs says: [error] ... is a child of [anotherError], what does that actually mean?
Does it mean it should be checkable with errors.Is? In such case, I cannot see where that "inheritance" (or better said: wrapping) is happening, but I might be missing something here.
There was a problem hiding this comment.
Does it mean it should be checkable with errors.Is?
Yeah, that's the intention. You're right though, "child" doesn't mean a clear thing here, and it should be phrased in a way where that's clear.
instance.go
Outdated
| func (i *Instance) Translate(to SyntacticVersion) (*Instance, TranslationLacunas) { | ||
| inst, lac, err := i.TranslateErr(to) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Could we remove this panic and return an error instead?
There was a problem hiding this comment.
that's what TranslateErr is for - though maybe it would be better to have a TranslateP that panics and Translate returns an error
There was a problem hiding this comment.
I like this idea because of the fact of leaving the simple version of Translate open for future releases, optionally moving towards deprecations instead of breaking changes - tho it's of course not critical because of the fact we're on v0.x 😄
sdboyer
force-pushed
the
sdboyer/translate-errors
branch
from
16fd8cb to
8070bba
Compare
| // | ||
| // Errors only occur in cases where lenses were written in an unexpected way - | ||
| // for example, not all fields were mapped over, and the resulting object is not | ||
| // concrete. All errors returned from this func will children of [terrors.ErrInvalidLens]. |
There was a problem hiding this comment.
| // concrete. All errors returned from this func will children of [terrors.ErrInvalidLens]. | |
| // concrete. All errors returned from this func will be children of [terrors.ErrInvalidLens]. |
There was a problem hiding this comment.
ahhhh whoops missed this, sorry
vmux/typed.go
Outdated
| trinst, lac := inst.Translate(sch.Version()) | ||
| trinst, lac, err := inst.Translate(sch.Version()) | ||
| if err != nil { | ||
|
|
There was a problem hiding this comment.
Shouldn't we do something here?
At the very very least leave a // TODO..., I guess!
sdboyer
force-pushed
the
sdboyer/translate-errors
branch
from
7a42ffc to
a0293d6
Compare
This PR introduces better error handling for
Translate.The function signature is remaining the same for
Translate, given the aspiration to eventually have it not return errors (as described in the new docs for it). It will still panic if errors occur.But, we also introduce
TranslateErr, which returns errors instead of panicking. All errors which could emerge fromTranslateErrare instances (errors.Is) of the newErrInvalidLenssentinel error value. (That won't actually work yet because we're still figuring out how to use cockroachdb/errors in the context of #164, but once we do this should "Just Work")The error sentinel disambiguation will be especially helpful in the context of vmuxers, where a non-nil error could either come from the validate step, or from the translate step.
Fixes #167