[
https://issues.apache.org/jira/browse/TINKERPOP-2525?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17514541#comment-17514541
]
ASF GitHub Bot commented on TINKERPOP-2525:
-------------------------------------------
FlorianHockmann commented on a change in pull request #1605:
URL: https://github.com/apache/tinkerpop/pull/1605#discussion_r838250544
##########
File path:
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Strategy/Decoration/SubgraphStrategy.cs
##########
@@ -53,6 +54,10 @@ public SubgraphStrategy() : base(JavaFqcn)
Configuration["edges"] = edges;
if (vertexProperties != null)
Configuration["vertexProperties"] = vertexProperties;
+
+ // hate to write this by default so only going to write it if it's
false which is the non-default value
+ if (!checkAdjacentVertices)
+ Configuration["checkAdjacentVertices"] = checkAdjacentVertices;
Review comment:
Really just a nit, but my IDE (Rider) is complaining that
`checkAdjacentVertices` will always be `false` here and suggests to change this
to `Configuration["checkAdjacentVertices"] = false;` to make this more obvious.
##########
File path:
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Strategy/Decoration/SubgraphStrategy.cs
##########
@@ -53,6 +54,10 @@ public SubgraphStrategy() : base(JavaFqcn)
Configuration["edges"] = edges;
if (vertexProperties != null)
Configuration["vertexProperties"] = vertexProperties;
+
+ // hate to write this by default so only going to write it if it's
false which is the non-default value
Review comment:
Are we 100% sure that `true` will stay the default? Otherwise, we should
maybe really just always add this so we don't at some point change the default
in Java to `false` and then forget to update this here.
##########
File path:
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Strategy/Decoration/SubgraphStrategy.cs
##########
@@ -43,8 +43,9 @@ public SubgraphStrategy() : base(JavaFqcn)
/// <param name="vertices">Constrains vertices for the <see
cref="ITraversal" />.</param>
/// <param name="edges">Constrains edges for the <see
cref="ITraversal" />.</param>
/// <param name="vertexProperties">Constrains vertex properties for
the <see cref="ITraversal" />.</param>
+ /// <param name="checkAdjacentVertices">Determines if filters are
applied to the adjacent vertices of an edge.</param>
public SubgraphStrategy(ITraversal vertices = null, ITraversal edges =
null,
- ITraversal vertexProperties = null)
+ ITraversal vertexProperties = null, bool checkAdjacentVertices =
true)
Review comment:
You could also change this to `bool? checkAdjacentVertices = null` and
then change the two lines below to:
```c#
if (checkAdjacentVertices != null)
Configuration["checkAdjacentVertices"] = checkAdjacentVertices.Value;
```
if you want this parameter to be optional and only set it if the user
supplied a value here, irrespective of whether this value is `true` or `false`.
But we can also keep this as is from my side.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
> Extend Gherkin tests to cover strategies
> ----------------------------------------
>
> Key: TINKERPOP-2525
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2525
> Project: TinkerPop
> Issue Type: Improvement
> Components: test-suite
> Affects Versions: 3.4.10
> Reporter: Stephen Mallette
> Priority: Minor
>
> We now have a much more advanced test suite in 3.5.0 for processing Gherkin
> tests. We should extend the Gherkin tests further to include tests for
> {{TraversalStrategy}} instances.
> Of note {{SubgraphStrategy}} class doesn't have a {{checkAdjacentVertices}}
> parameter in its constructor.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
