Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
Excellent - thanks for doing this. @jorgebay is the better person to handle
this review in full, but I'd like to offer a few points:
> Is it safe to assume that a returned value not containing a bulk property
was executed by an eval op?
I'm not sure I would call it "safe", but that is typically what happens. I
would say that it is not completely "safe" to do so because an `eval()` could
return anything and thus return a `BulkSet` and the user would expect that and
I guess it would unroll to individual items when you iterated which the user
wouldn't expect.
> Should every parameter be bound when building a dynamic script?
We refer to what you've done there as a `Translator` (i.e. something that
converts bytecode to the string representation of the Gremlin) and that's neat
that you have that. We don't implement that as a `toString()` on `Bytecode` but
perhaps that's ok though. FYI, here's what a `Translator` looks like in Java:
```groovy
String gremlin = GroovyTranslator.of("g").translate(bytecode)
```
I don't believe that our translators convert arguments to bindings. It
assumes them to be literals. We only construct bindings if the user constructs
them as first order `Bindings` in the Gremlin itself, but I don't know if we
have that concept here in javascript. For consistency sake, we might want to.
You can see the approach taken by Python to handle that as an example:
http://tinkerpop.apache.org/docs/current/reference/#_bindings
> Does the unit test need to be more thorough and check more varied
instructions and more complex glv queries/instructions?
On the JVM side we test the translators through the GLV suite (essentially
we use the JVM process test suite which is basically the same thing). Maybe
the same should be done here.
Separate from your questions, I'd offer that `g.eval()` isn't really in
line with how other GLV drivers submit scripts. While I'd agree it to be
convenient to have it work that way, I think it would be better to see that
working in a way more consistent with other GLVs.
---
