Closed desertkun closed 6 years ago
Merging #14 into master will increase coverage by
3.3%
. The diff coverage is88.23%
.
@@ Coverage Diff @@
## master #14 +/- ##
========================================
+ Coverage 67.79% 71.1% +3.3%
========================================
Files 16 16
Lines 1180 1263 +83
Branches 184 205 +21
========================================
+ Hits 800 898 +98
+ Misses 257 237 -20
- Partials 123 128 +5
Impacted Files | Coverage Δ | |
---|---|---|
v8py/jsfunction.cpp | 96% <88.23%> (+51.55%) |
:arrow_up: |
v8py/convert.cpp | 67.97% <0%> (+1.96%) |
:arrow_up: |
v8py/jsobject.cpp | 78.65% <0%> (+4.49%) |
:arrow_up: |
v8py/v8py.cpp | 75.88% <0%> (+7.98%) |
:arrow_up: |
v8py/polyfill.h | 100% <0%> (+25%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update d7e4e41...5b435eb. Read the comment docs.
I don't like the idea of adding a method to JSFunction or JSObject. What if JavaScript sets Test.new
to something?
I'd prefer just being able to say Test()
and have it use the new operator if you're working with a class. This isn't generally possible to find out, though. We could check whether there is a property called prototype
, but you can use new
with functions that don't have prototypes. There's a v8::Object::IsConstructor
method that seems promising, but I'm pretty sure it returns true for every user-defined function (since you can use new
on those).
Since that won't work, the interface I'd prefer is something like this:
from v8py import new
instance = new(Test, "world")
or maybe
instance = new(Test)("world")
Yeah, good point.
What's next? I can add a new commit into my bound_function_new
that changes the API into new(Test, "world")
OR, we can dismiss this pull request and start a new one, can we?
You could do either.
Okay, changed it to be context.new(Test, "world")
since context is required. I tried to pull one from Test
, but context is also required to do get Local<Value>
of the constructor function in the first place.
Also updated tests.
The reason a context is required by js_from_py
is it has to be able to handle Python objects as well as JavaScript objects, but new
doesn't need to handle Python objects. In fact, it can throw an exception if the argument isn't a JSFunction. So you can just do PyArg_ParseTuple(args, "O!", &js_function_type, &function)
. That also takes care of throwing a TypeError if it's the wrong type.
Could you implement this as a function on the v8py module?
Well, I was going to, but came into a question: what if you need to spawn a new object into context A
from a constructor function that belongs to context B
? How the new new
can handle that? I came into conclusion that explicitly specifying the context would solve the problem.
I can get rid of js_from_py
, though, and make it to use js_function_type
like you said, but I have to specify context anyway for the very reason above. Well, I can make function new(constructor, *args, context=None)
of the v8py module so you can specify one if you want but that's not obvious implementation and this project does lack documentation. So I am worried would someone ever find this feature?
I also can additionally add a new function into a module that simply uses the context of the constructor function in order to spawn an object. Are you okay with two implementations, one for explicitly specifying target context, and one without such?
What do you say?
V8 won't let you call a function or constructor from one context in a different context, the execution always happens in the context where the function was created. You can then put the resulting object in any context you like.
Please take my apology for taking so much of your time regarding this issue.
Finally, moved the method to the function to the v8py module.
Please take my apology for taking so much of your time regarding this issue.
Oh, it's no problem. Not sure if I should be apologizing for making you rewrite this 15 times :)
So you can do this: