Closed andydotxyz closed 5 years ago
I could not see any code using "env interface{}" so I replaced it. If that functionality needs to be maintained then I could add Environment.Data as interface{}?
I'm not averse to this, I guess it would be useful to make the library more useful for embedded users?
I guess there are two obvious comments:
INPUT
.PRINT 3 * 3
and assert the output is 9.Great! I agree on both points. This is already tested - I added the assertion to the PRINT and DUMP tests that existed already - do you want more?
I will look at the INPUT next - I think it should be moved to builtin now that it is possible.
I could not see any code using "env interface{}" so I replaced it.
Take a look at goserver/main.go
. That uses env
to invoke allowing state to be maintained between save
and other code. It is a contrived example, but without that you can't have any builtins that set/update global state, so it is worth keeping for embedded users.
If that functionality needs to be maintained then I could add Environment.Data as interface{}?
That works, I think.
Great! I agree on both points. This is already tested - I added the assertion to the PRINT and DUMP tests that existed already - do you want more?
No, that's fine. I read quickly and missed them. Perfect :)
I will look at the INPUT next - I think it should be moved to builtin now that it is possible.
Sadly you can't make it a built-in, with the way they work at the moment. (Ditto READ for the same reason)
If you move the code to a builtin and call:
10 INPUT "Steve", A
The builtin-handler will be called with two arguments:
What the INPUT handler needs is to be able to be passed a reference to the variable, so it can update the value. (+/- A vs. A$).
I think I might have documented that at some point, but it's a blocker to move it to builtin, and out of the core.
Anyway I'll check back tomorrow. Thanks for your contribution (and your fast replies! Bedtime here now though.)
I think that addresses your comments, thanks for the input. Goserver seems to be working to the best of my knowledge.
Merged, thanks again for your contribution :)
Requires a concrete Environment type for builtin. Env was not used for anything so this seemed safe...