[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[FlorianHockmann]

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
Since I also don't have a strong opinion on that matter, I just went ahead 
and only introduced the `ILambda` interface to make the implementation internal 
as you suggested.


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[jorgebay]

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
I would prefer a generic `ILambda` for string based lambdas (groovy and 
python) but I don't have an strong opinion about it.


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[FlorianHockmann]

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
> Can we make Lambda class static and make the Lambda.Groovy() and 
Lambda.Python() return an ILambda interface?

Sure, making the implementation internal makes perfect sense to me.

But what do you think about the question of whether we want to return a 
general `ILambda` interface vs specific `IFunction` interfaces and so on?

So first approach (basically the current one in this PR):
```cs
public static class Lambda
{
public static ILambda Groovy(string expression)
{
return new StringBasedLambda(expression, "gremlin-groovy");
}

public static ILambda Python(string expression)
{
return new StringBasedLambda(expression, "gremlin-python");
}
}
```

alternative approach:

```cs
public static class Lambda
{
public static string DefaultLanguage { get; set; } = "gremlin-groovy";

public static IFunction Function(string expression, string language)
{
return new StringBasedLambda(expression, language);
}

public static IPredicate Predicate(string expression, string language)
{
return new StringBasedLambda(expression, language);
}

// ...

public static IPredicate Predicate(string expression)
{
return Predicate(expression, DefaultLanguage);
}

// ...
}
```


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[jorgebay]

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
The implementation looks great and the final `GraphTraversal` and 
`GraphTraversalSource` APIs make a lot of sense, like:

```csharp
public GraphTraversal Branch (IFunction function) {}
```

This enables us that if we wanted to in the future, we could implement a 
lambda in C# that takes a `Func`, like:

```csharp
IFunction fn = Lambda.GetFunction(text => text.length)
int traversal.Branch(fn).Next();
```

Just 1 suggestion:
Can we make `Lambda` class `static` and make the `Lambda.Groovy()` and 
`Lambda.Python()` return an `ILambda` interface?

```csharp
public interface ILambda : IFunction, IBiFunction, IPredicate,
   IUnaryOperator, IBinaryOperator, IComparator, 
IConsumer, ISupplier
```

```csharp
// Make the implementation internal
internal class StringBasedLambda: ILambda
{
  public string LambdaExpression { get; }
  public string Language { get; }
  public object Arguments { get; }
  public Lambda(string expression, string language)
  {
// ...
  }
}
```

Then, the serializers could take an `ILambda`: `{typeof(ILambda), new 
LambdaSerializer()}`


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[FlorianHockmann]

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
I just rebased on `tp32` and hopefully also addressed your suggestions. 
`Lambda` now implements a bunch of interfaces that mirror the Java functional 
interfaces from the traversal API:
```cs
public class Lambda : IFunction, IBiFunction, IPredicate, IUnaryOperator, 
IBinaryOperator,
IComparator, IConsumer, ISupplier
```
This should make it easier for users to see where lambdas can be used. I 
also added a short note to the documentation about these interfaces for lambdas.

With all these new interfaces there is now just one Java type left for 
which we don't have a .NET equivalent: `VertexProgram`. So the next release 
will again increase type-safety of the traversal API a lot.

We are now down to just 5 ignored scenarios for `tp32` 🎉

@jorgebay, you linked the Java `Lambda` interface in one of your comments. 
The same file also contains classes like `TwoArgLambda` that are very similar 
to the `Lambda` class I implemented in this PR, but they use [different methods 
for the different 
interfaces](https://github.com/apache/tinkerpop/blob/3.2.7/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/function/Lambda.java#L158)
 like:
```java
public static  Consumer consumer(final String lambdaSource,
final String lambdaLanguage)
```
Do we also want to have similar methods in Gremlin.Net? It would be very 
easy to implement and I can image that just having a `Lambda` object could lead 
to some confusion as to where it can be used.


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[spmallette]

Github user spmallette commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
I'll let @jorgebay dig into the c# stuff here, but from my view this looks 
pretty good and it's nice to see this feature fully supported now. Great to 
know that all those feature tests pass.  It's too bad that this doesn't match 
java too well:

> Unfortunately, it's not possible to use this new Lambda type for 
arguments in the traversal API

but i guess there's not much we can do about that.  Perhaps, this 
limitations should be explained in the documentation for "The Lambda Solution"? 
From a documentation perspective, I think that (1) this is a big enough 
improvement that you should make note of it in the upgrade documentation and 
(2) this needs a changelog entry before you merge.

Otherwise, all tests pass with `docker/build.sh -t -n -i`

VOTE +1


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[FlorianHockmann]

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
> The discussion on TINKERPOP-1857 shows that we actually still need the 
functionality to ignore tests. So I'll have to update this PR to keep that 
functionality in.

I just did this update. So the PR is ready for review again.


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[FlorianHockmann]

Github user FlorianHockmann commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
The discussion on 
[TINKERPOP-1857](https://issues.apache.org/jira/browse/TINKERPOP-1857) shows 
that we actually still need the functionality to ignore tests. So I'll have to 
update this PR to keep that functionality in.


---

[GitHub] tinkerpop issue #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net

[jorgebay]

Github user jorgebay commented on the issue:

https://github.com/apache/tinkerpop/pull/792
  
I didn't have time to review this one, I hope I will be able to look into 
it by the end this week.


---