Pass through JSON #12
Conversation
sparkprime
force-pushed
the
json_pass_through
branch
from
6ba40a3 to
09c7c8c
Compare
|
About unit tests and coverage: For the interpreter I think we're good with end-to-end tests. For the desugarer, some unit tests are probably a good idea for development purposes. I'm pretty confident we can get good coverage all the way through with just end-to-end tests (or if not, we can construct more end-to-end tests to get good coverage). However for development, it's nice to have tests that validate particular areas of the code. However, I haven't done this yet. |
|
I've started looking at this but don't have a ton of bandwidth right now (moving, vacation). Let me know if you need me to hurry up. |
|
Nah there's no particular pressure on this. I intend to integrate Jsonnet with helm but for a first release of Helm it's fine to use the C++ code, and then we can swap it out for the Go version later when security becomes a concern. I am quite happy with the progress on go-jsonnet so far. |
| // TODO(jbeda): Add constructor helpers here | ||
| // TODO(jbeda): Add the remaining constructor helpers here | ||
|
|
||
| func astObjectFieldLocal(methodSugar bool, id *identifier, ids identifiers, trailingComma bool, body astNode) astObjectField { |
There was a problem hiding this comment.
linter: This function is currently unused.
|
My editor is using gometalinter so that might be useful for annoying to remember golang stuff.
Looking at code more carefully now. Hopefully I'll be done by end of day. Sorry it took me so long to pick this up again. |
| // TODO(dcunnin): Remove all uses of unimplErr. | ||
| unimplErr := makeStaticError(fmt.Sprintf("Desugarer does not yet implement ast: %s", reflect.TypeOf(ast)), *ast.Loc()) | ||
|
|
||
| var err error // Make all recursive calls of the form err = desugar, rather than some being err := desugar |
There was a problem hiding this comment.
I think you can define this variable as part of the return --
func foo(...) (err error)Then you can either just return or still return err.
|
LGTM -- I'm a noob here but it looks reasonable. |
|
Thanks for review. Took me over a week to actually find time to act on it :) Do you use vim? I just tried gometalinter on the commandline but had problems with errors like this: and then tons of other errors that I suspect are a knock-on from that. |
|
@sparkprime -- I'm using Atom for this project -- just switched for fun. The go support seems pretty good. Sorry I can't help you with vim. gometalinter isn't a must -- it is probably overkill. Just thought I'd point it out since some of the stuff looks useful. |
So the rabbit hole was deeper than I thought... But I believe this is all that is needed for objects, arrays, and strings. No tests. No regrets!
The desugarer is fully laid out but has lots of holes (marked with TODOs). There is no static analysis yet! This will become very important when evaluating functions and thunks, because it is the static analysis phase that fills in the freeVariables for each astNode. Without that, variable lookup will fail if the variable is captured from the environment (instead of being a direct parameter).
Simple objects are added to the interpreter, these are objects formed by { } and not with a comprehension and or by combining two objects with the + operator (a mixin). I left space to add the code for these, but it wasn't necessary for passing through JSON.
The stack is very different to in the C++ version in that it is only used for variable bindings and reconstructing stack traces in errors. It is not used to store partial execution. Likewise the inner interpreter loop is very different and much more readable as it uses simple recursion.
Fixes #11