thema icon indicating copy to clipboard operation
thema copied to clipboard

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

Open mildwonkey opened this issue 2 years ago • 4 comments

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

mildwonkey avatar May 16 '23 19:05 mildwonkey

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

joanlopez avatar May 18 '23 08:05 joanlopez

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 avatar May 18 '23 14:05 mildwonkey

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)

mildwonkey avatar May 22 '23 18:05 mildwonkey

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):

  • In lenses, there must exist exactly one lens for each expected pair of version numbers
  • In each lens result, there must exist at least one "assignment" for every field in the to schema? (handwave handwave optional fields)

sdboyer avatar May 23 '23 09:05 sdboyer