caketop / python-starlark-go

🐍 Python bindings for starlark-go 🐍
https://python-starlark-go.readthedocs.io/
Apache License 2.0
20 stars 7 forks source link

Make ConversionErrors more useful #167

Closed jordemort closed 1 year ago

jordemort commented 1 year ago

Closes #143

jordemort commented 1 year ago

@colindean LMK what you think of this; the error messages now call out the index or key (and the case of conversion from Starlark to Python, the actual value since we have a handy representation) that fails the conversion. This will make the errors pretty long for deeply nested containers, since each parent prepends its stamp on to the error, but I'd rather have useful errors than pretty ones.

jordemort commented 1 year ago

(Examples of what the new errors look like in practice can be found in the new tests)

tadamcz commented 1 year ago

Thanks for doing this!

Definitely agree it's a huge improvement from:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x1050f92f0]

goroutine 17 [running, locked to thread]:
main.Starlark_set_globals(0x104a47110, 0x1047a0398, 0x104b21dc0)
    /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmpn1y1slrv/src/github.com/caketop/python-starlark-go/python_globals.go:120 +0x2d0

Might be worth releasing on pypi, but up to you. (I'm now pulling the dependency directly from your repo).

jordemort commented 1 year ago

@tadamcz I thought I was going to barrel ahead to a 2.0 with support for structs and compiled expressions but I got bogged down in how I want to actually expose that stuff on the Python side; still need to think about it some more.

I'll kick a 1.1 out to PyPI in the next couple days since main is significantly better/more correct that what's published now.