Add backtraces to error starlark command output

[thomasf]

and update starlark depdency.

[thomasf]

The same starlark version as in drone/drone-convert-starlark#8

[tboerger]

IMHO it should not print multiline errors, but maybe @bradrydzewski got a different opinion.

[thomasf]

IMHO it should not print multiline errors, but maybe @bradrydzewski got a different opinion.

It's often completely useless if it doesn't though. You can get errors like like update: got NoneType, want iterable which could be literally anywhere in your script where you ran update on a dictionary (or a lot of similar cases). If you have function calling other functions you very much need the call stack context to understand why some variable might not be set.

This change probably saved me hours a couple of weeks ago when I were writing a lot of drone starlark. Just before I looked into this I had to read a starlark source for almost an hour just to figure out why/where a certain error had occurred.

[thomasf]

Why should it not print multi line errors? I mean the newlines can be replaces by concatenating with some other character instead but it's harder to read and less user friendly

[thomasf]

For reference there is a test in the drone-convert-starlark repository where the error format is tested for..

https://github.com/drone/drone-convert-starlark/pull/8/files#diff-00aa2d840f6c83c3c66e0078bbc6ff42

[tboerger]

A library should not print multiline errors, what this package is. That should up to the caller, so IMHO it should return an error type that gets properly handled by the caller.

[thomasf]

I guess that prettyStarlarkError could log the stack trace instead of turning it into an error, that would mean that the error will be written twice since the error will be written later in the execution anyways.

I guess that an important distinction is that drone-cli isn't a library, it's an application and it is handling the error in an end user friendly way.

[thomasf]

In the case of the starlark conversion plugin the plugin interface maybe has to be rewritten to support this since the error that is returned is just written to the http response directly. It needs to maybe have check for it an error writer or something like that.. maybe if something like this was supported via an type assertion the http plugin handler can choose the verbose error and it would be backwards compatible for normal "short" errors.

interface {
  Error () string
  VerboseError() string
}

[thomasf]

I am waiting for suggestions how to change this pull request. I have suggested one solution myself but it's no use for me changing it without additional input. Just saying no without suggesting options is't particulary helpful to me as an contributor and to the end users since working with non trivial starlark scripts currently has horrible UX.

[tboerger]

drone-cli itself is not a library, but this package is a library, so imho the calling code should handle a trace print.

[thomasf]

I'm still a little bit confused.. While it is not inside an internal package this starlark package feels a lot more like supporting package to drone-cli than an library (which by definition is meant for reuse), for one thing it lacks well written documentation and for most functions documentation at all and there are no tests which IMO is more important than caring about if errors have multi line strings in their returns.

thats enough with the philosophical discussion for now

The only package that imports this starlark package seems to be the main function in drone-cli ( https://godoc.org/github.com/drone/drone-cli/drone/starlark?importers ) which has a very generic look to it. So you think I should add starlark specific error handling code to the return function at the end of main.go ?

[thomasf]

Is this better?

[bradrydzewski]

thanks for making the updates, this looks good to me.