[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17844607#comment-17844607
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
FlorianHockmann merged PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841275#comment-17841275
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
vkagamlyk commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2079624005
VOTE +1
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17841266#comment-17841266
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
FlorianHockmann commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2079581200
The problem is however that the Gherkin tests already include this step but
they already passed before my change here as @spmallette noted in the issue
description.
This got me curious however as I was wondering how this could actually work
without an overload taking a dictionary in C# since I couldn't get it to work
in C# without my change here. (I did write a test case when I implemented this.
I only deleted it before committing to not impact other tests.)
Stephens assumption was:
> It works because it probably piggybacks on property(object?, object?,
objects[]?) which has all nullable arguments.
Turns out that this is not the case. The `DotNetTranslator` produces a C#
traversal with individual `Property` steps for each map entry:
https://github.com/apache/tinkerpop/blob/e3ced38b2108f3edb29f11fbfa4f6950427bcbd9/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs#L539
So, as a next attempt I wanted to change the translator to instead produce a
C# traversal using the new `Dictionary` overload.
But then I learned that that's now even possible since Gremlin-Java already
produces Bytecode without this overload:
https://github.com/apache/tinkerpop/blob/e3ced38b2108f3edb29f11fbfa4f6950427bcbd9/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java#L3221-L3234
and the translator only gets the Bytecode produced from the traversal.
So I don't see any elegant way to test this using Gherkin tests without
changing the Gremlin-Java overload (which is a bigger change than what makes
sense here in my opinion). We could add a hard-coded translation for one
feature test to ensure that the traversal in C# will use the new overload, but
I don't really like maintaining such hard-coded translations.
However, this got me to the idea to instead use the translation from C# to
Groovy for a test. I just pushed an updated commit with such a test. The test
does not compile without the added overload and the translation also includes
the successful construction of Bytecode from the traversal as the translator
also uses the Bytecode.
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17840573#comment-17840573
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
xiazcy commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2075810348
I believe for Gherkin tests we have the empty graph that can be modified,
since it gets reset for each test. This does mean adding a Feature test that
will run for all GLVs.
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17840334#comment-17840334
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
FlorianHockmann commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2074375930
> nit: no test coverage
Yes, I wanted to add a test for this, but the problem is that it's
manipulating data in the graph. I don't know how to test this with our current
test setup without impacting other tests. If we could start a container with
Gremlin Server for just this test, then we could easily test it. But our
current setup uses the same container for all tests with the same graph. If we
write to this graph in one test, then it will impact other tests.
Another option would be to write a unit test which only checks the generated
Bytecode, but I think such tests have little value, especially when we want to
get rid of Bytecode altogether in a future release.
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17840211#comment-17840211
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
vkagamlyk commented on PR #2565:
URL: https://github.com/apache/tinkerpop/pull/2565#issuecomment-2073301294
nit: no test coverage
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (TINKERPOP-3035) Add explicit property(IDictionary) for .NET
[
https://issues.apache.org/jira/browse/TINKERPOP-3035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17838187#comment-17838187
]
ASF GitHub Bot commented on TINKERPOP-3035:
---
FlorianHockmann opened a new pull request, #2565:
URL: https://github.com/apache/tinkerpop/pull/2565
https://issues.apache.org/jira/browse/TINKERPOP-3035
VOTE +1
> Add explicit property(IDictionary) for .NET
> ---
>
> Key: TINKERPOP-3035
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3035
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
>Affects Versions: 3.6.6
>Reporter: Stephen Mallette
>Priority: Minor
>
> There is no {{property(IDictionary)}} method for .NET to
> accompany the {{property(Map)}} step. It works because it probably piggybacks
> on {{property(object?, object?, objects[]?)}} which has all nullable
> arguments. The explicit overload with the {{IDictionary}} should be added.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
