Support for invoking a function value #100
Conversation
|
This is a work in progress -- still need to add more tests -- but let me know if I'm not on the right track. Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
==========================================
+ Coverage 96.53% 96.61% +0.08%
==========================================
Files 11 12 +1
Lines 404 414 +10
==========================================
+ Hits 390 400 +10
Misses 9 9
Partials 5 5
Continue to review full report at Codecov.
|
|
OK, this should have 100% test coverage. The current failure is about a missing FOSSA key(?) |
function.go
Outdated
| } | ||
|
|
||
| // Call this JavaScript function with the given arguments. | ||
| func (fn *Function) Call(args []*Value) (*Value, error) { |
There was a problem hiding this comment.
Can we change the args to be the Valuer interface? Then you can use any Value type. ie Object etc
function.go
Outdated
| var rtn C.RtnValue | ||
| if len(args) == 0 { | ||
| rtn = C.FunctionCall(fn.ptr, C.int(0), (*C.ValuePtr)(nil)) | ||
| } else { |
There was a problem hiding this comment.
Is this else statement needed? The range would still work with a empty array?
If we need to pass the nil pointer can we return in the above if statement to avoid the extra branching caused by the else
There was a problem hiding this comment.
That was something I found by writing the test -- to get the *C.ValuePtr, I have to take the value of the first element, and that is an illegal index on an empty slice.
Updated to avoid calling FunctionCall in two different branches.
|
Thanks for the review. Comments addressed, and tests pass locally. |
function_test.go
Outdated
| failIf(t, err) | ||
|
|
||
| fn, _ := addValue.AsFunction() | ||
| resultValue, err := fn.Call([]v8go.Valuer{arg1, arg1}) |
There was a problem hiding this comment.
This is looking great @robfig.
Sorry to be a pain, but looking at this I'm wondering if a variadic function would be more idiomatic? Then you could do:
fn.Call(arg1, arg2, etc)
There was a problem hiding this comment.
No problem at all -- agreed that it looks cleaner. Updated
|
Added & updated! |
|
Perfect @robfig |
|
Sounds good -- not sure if you expect me to merge, but I don't have access to do that despite your approval. I'll leave it to you. |
|
I'll merge shortly. |
Fixes #95