Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ConversionErrors more useful #167

Merged
merged 7 commits into from
Mar 18, 2023
Merged

Make ConversionErrors more useful #167

merged 7 commits into from
Mar 18, 2023

Conversation

jordemort
Copy link
Contributor

  • Subclass into ConversionToPythonFailed and ConversionToStarlarkFailed
  • Make messages much more informative and indicate which key or index failed
  • If conversion fails because of a Python exception, add that exception as the source of ours

Closes #143

- Subclass into ConversionToPythonFailed and ConversionToStarlarkFailed
- Make messages much more informative and indicate which key or index failed
- If conversion fails because of a Python exception, add that exception as the source of ours

Closes #143
@jordemort
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Copy link
Contributor

@colindean colindean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a big improvement!

@jordemort jordemort merged commit 4d80515 into main Mar 18, 2023
@jordemort jordemort deleted the better-conversion-errors branch March 18, 2023 18:43
@tadamcz
Copy link
Contributor

tadamcz commented Mar 30, 2023

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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't tell what starlark Builtin cannot be converted to Python
3 participants