Replace Bytecode with GremlinLang in .NET driver#3309
Replace Bytecode with GremlinLang in .NET driver#3309xiazcy wants to merge 3 commits intodotnet-httpfrom
Conversation
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversalSource.cs
Outdated
Show resolved
Hide resolved
| if (name[0] == '_') | ||
| throw new ArgumentException($"Invalid GValue name {name}. Should not start with _."); | ||
|
|
||
| if (!char.IsLetter(name[0])) |
There was a problem hiding this comment.
Nit: this is probably just an || with (name.Length == 0)
There was a problem hiding this comment.
I think I'll leave it at this, mainly b/c if we wanted to keep a specific exception message for _ cases, we'll have to keep the checks separate.
| CancellationToken cancellationToken = default) | ||
| { | ||
| await writer.WriteNonNullableValueAsync(value.Bytecode, stream, cancellationToken).ConfigureAwait(false); | ||
| throw new NotSupportedException("Traversal serialization via GraphBinary is no longer supported. Use GremlinLang instead."); |
There was a problem hiding this comment.
Why not just remove this from the interface altogether?
There was a problem hiding this comment.
This is just an interim change to allow compilation when Bytecode class was removed. Throwing is just a bit more elegant.
...in.Net.IntegrationTest/Process/Traversal/GremlinLangGeneration/GremlinLangGenerationTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Nit: the naming of each unit test in this file is not in line with the rest of this repo. Tests generally named with a "should...", not really a big problem but it makes it difficult to understand what exactly each test is trying to test.
There was a problem hiding this comment.
I'm current leaning towards leaving the test name consistent across GLVs. We could revisit and change the names later.
| public void g_AddV_test_Property_Cardinality_Set_p1_10() | ||
| { | ||
| Assert.Equal("g.addV(\"test\").property(Cardinality.set,\"p1\",10)", | ||
| _g.AddV("test").Property(Cardinality.Set, "p1", 10).GremlinLang.GetGremlin()); |
There was a problem hiding this comment.
Could you add some more complex cases for Cardinality in property such as
g.V(1).property(list, ['age': single(36), 'city': 'wilmington', 'state': 'delaware'])
Sourced from reference docs: https://tinkerpop.apache.org/docs/current/reference/#property-step
There was a problem hiding this comment.
Added. Also had to add a Property(Cardinality cardinality, IDictionary<object, object> map) overload for .NET for the case, as apparently it's not as flexible as Java with the existing signatures. Do note that the GremlinLang itself becomes just property(cardinality, key, value) chains, cause that's how the step adds itself in GraphTraversal.
| if (arg is short shortVal) | ||
| return $"{shortVal}S"; | ||
| if (arg is int intVal) | ||
| return intVal.ToString(CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
Just curious, was there an issue that requires CultureInfo.InvariantCulture here?
There was a problem hiding this comment.
It's a .NET-specific concern. CultureInfo.InvariantCulture is required because without it, ToString() uses the current thread's culture, which could produce locale-specific decimal separators (e.g., comma instead of period in German/French locales). Gremlin-lang requires invariant number formatting.
|
VOTE +1 |
This PR replaces Bytecode with GremlinLang in the .NET Gremlin driver, aligning gremlin-dotnet with the 4.0 format already implemented in Java, Python, and Go.
3 new files were added, GremlinLang.cs, GValue.cs, and GremlinLangTests.cs. All files with Bytecode reference were replaced with GremlinLang. Bytecode specific files as well as all Groovy translator files were deleted.
Note this PR is opened against a feature branch for the 4.0 .NET driver, which will be merged into master after all components are completed. As well, current test are unit tests only, additional changes may be needed once HTTP protocol and serialization are properly implemented with Gherkin tests enabled.