You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/01/18 17:32:20 UTC
[GitHub] [tinkerpop] mikepersonick opened a new pull request #1551: Fixing .NET test failures and adding Gherkin support for VertexProperty.
mikepersonick opened a new pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551
https://issues.apache.org/jira/browse/TINKERPOP-2688
https://issues.apache.org/jira/browse/TINKERPOP-2689
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#discussion_r787727496
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Remote/RemoteStrategyTests.cs
##########
@@ -1,4 +1,4 @@
-#region License
Review comment:
Is this a known thing? Rider was not able to compile those source files for me bc of it. Is there an IDE setting that can fix it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann merged pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
FlorianHockmann merged pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#discussion_r787730911
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs
##########
@@ -133,6 +137,70 @@ public ScenarioData(IMessageSerializer messageSerializer)
}
}
+ private static IDictionary<string, VertexProperty> GetVertexProperties(GraphTraversalSource g)
+ {
+ try
+ {
+ /*
+ * This closure will turn a VertexProperty into a triple string of the form:
+ * "vertexName-propKey->propVal"
+ *
+ * It will also take care of wrapping propVal in the appropriate Numeric format. We must do this in
+ * case a Vertex has multiple properties with the same key and number value but in different numeric
+ * type spaces (rare, but possible, and presumably something we may want to write tests around).
+ */
+ string groovy = @"
+ { vp ->
+ def result = vp.element().value('name') + '-' + vp.key() + '->'
+ def value = vp.value()
+ def type = ''
+ switch(value) {
Review comment:
Some poor soul has to do the same in Javascript and Python...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on a change in pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#discussion_r787730911
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs
##########
@@ -133,6 +137,70 @@ public ScenarioData(IMessageSerializer messageSerializer)
}
}
+ private static IDictionary<string, VertexProperty> GetVertexProperties(GraphTraversalSource g)
+ {
+ try
+ {
+ /*
+ * This closure will turn a VertexProperty into a triple string of the form:
+ * "vertexName-propKey->propVal"
+ *
+ * It will also take care of wrapping propVal in the appropriate Numeric format. We must do this in
+ * case a Vertex has multiple properties with the same key and number value but in different numeric
+ * type spaces (rare, but possible, and presumably something we may want to write tests around).
+ */
+ string groovy = @"
+ { vp ->
+ def result = vp.element().value('name') + '-' + vp.key() + '->'
+ def value = vp.value()
+ def type = ''
+ switch(value) {
Review comment:
It was the same in Java. Now some poor soul has to do the same in Javascript and Python...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#issuecomment-1016418256
Adding `VertexProperty` to the test suite is most excellent - thanks.
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#issuecomment-1016310534
Thanks for this great contribution to Gremlin.NET, @mikepersonick! I just added two nitpick comments about C# style, but this is good to go from my side with or without addressing them.
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#discussion_r787737917
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Remote/RemoteStrategyTests.cs
##########
@@ -1,4 +1,4 @@
-#region License
Review comment:
I at least remember that we had some problems with this already in the past. I think some IDE (maybe Visual Studio) added the BOM byte to files which resulted in problems for others. I think we should remove it everywhere.
We could add an `.editorconfig` where we specify the charset as `utf-8` for all `*.cs` files. Most editors / IDEs support `.editorconfig` (a plugin might be necessary though) which should ensure that the BOM byte doesn't get added back. We can also use a GitHub action to enforce that the `.editorconfig` is honored.
This issue contains more details: editorconfig/editorconfig#297.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1551: Fixing .NET test failures and adding Gherkin/.NET support for VertexProperty.
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1551:
URL: https://github.com/apache/tinkerpop/pull/1551#discussion_r787486627
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs
##########
@@ -133,6 +137,70 @@ public ScenarioData(IMessageSerializer messageSerializer)
}
}
+ private static IDictionary<string, VertexProperty> GetVertexProperties(GraphTraversalSource g)
+ {
+ try
+ {
+ /*
+ * This closure will turn a VertexProperty into a triple string of the form:
+ * "vertexName-propKey->propVal"
+ *
+ * It will also take care of wrapping propVal in the appropriate Numeric format. We must do this in
+ * case a Vertex has multiple properties with the same key and number value but in different numeric
+ * type spaces (rare, but possible, and presumably something we may want to write tests around).
+ */
+ string groovy = @"
Review comment:
(nitpick) Really not important, but you can most of the time just use `var` instead of the concrete type of a variable as that can be interfered from its initialization. So, `var groovy = [...]` here.
This makes it a bit nicer to read in my opinion.
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs
##########
@@ -133,6 +137,70 @@ public ScenarioData(IMessageSerializer messageSerializer)
}
}
+ private static IDictionary<string, VertexProperty> GetVertexProperties(GraphTraversalSource g)
+ {
+ try
+ {
+ /*
+ * This closure will turn a VertexProperty into a triple string of the form:
+ * "vertexName-propKey->propVal"
+ *
+ * It will also take care of wrapping propVal in the appropriate Numeric format. We must do this in
+ * case a Vertex has multiple properties with the same key and number value but in different numeric
+ * type spaces (rare, but possible, and presumably something we may want to write tests around).
+ */
+ string groovy = @"
+ { vp ->
+ def result = vp.element().value('name') + '-' + vp.key() + '->'
+ def value = vp.value()
+ def type = ''
+ switch(value) {
Review comment:
Wow, a lot of effort to get these scenarios to work in .NET 🙈
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Remote/RemoteStrategyTests.cs
##########
@@ -1,4 +1,4 @@
-#region License
Review comment:
Hmm, the good old BOM byte. Maybe we should add an `.editorconfig` to prevent these from being added back again?
(Not suggesting that something like that should be added as part of this PR, just something that came to my mind while seeing this here.)
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/ScenarioData.cs
##########
@@ -133,6 +137,70 @@ public ScenarioData(IMessageSerializer messageSerializer)
}
}
+ private static IDictionary<string, VertexProperty> GetVertexProperties(GraphTraversalSource g)
+ {
+ try
+ {
+ /*
+ * This closure will turn a VertexProperty into a triple string of the form:
+ * "vertexName-propKey->propVal"
+ *
+ * It will also take care of wrapping propVal in the appropriate Numeric format. We must do this in
+ * case a Vertex has multiple properties with the same key and number value but in different numeric
+ * type spaces (rare, but possible, and presumably something we may want to write tests around).
+ */
+ string groovy = @"
+ { vp ->
+ def result = vp.element().value('name') + '-' + vp.key() + '->'
+ def value = vp.value()
+ def type = ''
+ switch(value) {
+ case { !(it instanceof Number) }:
+ return result + value
+ case Byte:
+ type = 'b'
+ break
+ case Short:
+ type = 's'
+ break
+ case Integer:
+ type = 'i'
+ break
+ case Long:
+ type = 'l'
+ break
+ case Float:
+ type = 'f'
+ break
+ case Double:
+ type = 'd'
+ break
+ case BigDecimal:
+ type = 'm'
+ break
+ case BigInteger:
+ type = 'n'
+ break
+ }
+ return result + 'd[' + value + '].' + type
+ }
+ ";
+
+ IFunction lambda = Lambda.Groovy(groovy, 1);
+
+ IDictionary<string, VertexProperty> dict = g.V().Properties<VertexProperty>().Group<string, VertexProperty>()
Review comment:
Same nit here basically:
```suggestion
var dict = g.V().Properties<VertexProperty>().Group<string, VertexProperty>()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org