search

grafana / thema

A CUE-based framework for portable, evolvable schema
Apache License 2.0
229 stars 12 forks source link

feature request: add error return value to `Translate()` #151

Open mildwonkey opened 1 year ago

mildwonkey commented 1 year ago

Translate() does not currently return an error, but there are cases in which it may panic, so we should return an error when issues are encountered.

https://github.com/grafana/thema/blob/e1d0481fa785f07a2aadac64d848e11abe5e83b6/instance.go#L152

joanlopez commented 1 year ago

Could you provide any specific example to reproduce this issue, please? 🙏🏻

mildwonkey commented 1 year ago

I CAN NOW! https://github.com/mildwonkey/thema-examples/tree/main/translate

There's a repo full repo in there, but it's as simple as calling Translate on a schema version that doesn't exist

mildwonkey commented 1 year ago

Another case when Translate() panics is if there are no lenses defined:

panic: #Translate.out.steps._accum.1._lens: index out of range [0] with length 0:
    /github.com/grafana/thema/translate.cue:104:31

That specific issue is similar to what I referenced https://github.com/grafana/thema/issues/152 (though when I opened that issue I wasn't getting a panic, just silent failure - I'm not sure what was different but I'll include a repro if it comes up again)

sdboyer commented 1 year ago

if there are no lenses defined:

yup, it's another invariant that needs checking. "Lens completeness" is its whole own invariant area. I'm guessing we'll be able to write checkers in stages. From where we are now, the following two checks are clearly necessary (but not sufficient):