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/09/13 18:44:11 UTC
[GitHub] [tinkerpop] vkagamlyk opened a new pull request, #1808: Mid-traversal E()
vkagamlyk opened a new pull request, #1808:
URL: https://github.com/apache/tinkerpop/pull/1808
- Added support for mid-traversal E()
- Added unit tests
- Added Cucumber tests
- Updated doc
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r972368829
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
Should I propagate these changes to `3.5-dev` and `3.6-dev`?
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969989180
##########
docs/src/dev/future/index.asciidoc:
##########
@@ -49,7 +49,6 @@ line of 22H2.
* Add support for traversals as parameters for `V()`, `is()`, and `has()` (includes `Traversal` arguments to `P`)
* Geospatial support for TinkerPop (link:++https://lists.apache.org/list?dev@tinkerpop.apache.org:2021-7:DISCUSS%20geo-spatial++[DISCUSS Thread])
-* Add mid-traversal `E()` support
Review Comment:
Added ticket https://issues.apache.org/jira/browse/TINKERPOP-2798
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969984611
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
ugh - that's too bad....
--
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 diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r971611811
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
I would do it in all affected classes to stay consistent.
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970085795
##########
docs/src/dev/future/index.asciidoc:
##########
@@ -49,7 +49,6 @@ line of 22H2.
* Add support for traversals as parameters for `V()`, `is()`, and `has()` (includes `Traversal` arguments to `P`)
* Geospatial support for TinkerPop (link:++https://lists.apache.org/list?dev@tinkerpop.apache.org:2021-7:DISCUSS%20geo-spatial++[DISCUSS Thread])
-* Add mid-traversal `E()` support
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1808/commits/13501c50233d1692d2cd3dd8185e61e9050f8fe3
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -1236,8 +1236,8 @@ g.V()
[[e-step]]
=== E Step
-The `E()`-step is meant to read edges from the graph and is usually used to start a `GraphTraversal`. Unlike the related
-<<v-step,V()>> it cannot be used mid-traversal.
+The `E()`-step is meant to read edges from the graph and is usually used to start a `GraphTraversal`, but can also
+be used mid-traversal.
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1808/commits/13501c50233d1692d2cd3dd8185e61e9050f8fe3
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r973245936
##########
gremlin-dotnet/src/Gremlin.Net/Gremlin.Net.csproj:
##########
@@ -67,7 +67,7 @@ NOTE that versions suffixed with "-rc" are considered release candidates (i.e. p
<RepositoryUrl>https://github.com/apache/tinkerpop</RepositoryUrl>
<PublishRepositoryUrl>true</PublishRepositoryUrl>
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
- <EnablePackageValidation>true</EnablePackageValidation>
+ <EnablePackageValidation>false</EnablePackageValidation>
Review Comment:
updated in https://github.com/apache/tinkerpop/pull/1808/commits/5028d7a6078f828a5b8d69426464e67bb5853f22
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r972249988
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
renamed in https://github.com/apache/tinkerpop/pull/1808/commits/fc3480cc5a217a215e48ae13396082effd59085a
--
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 #1808: TINKERPOP-2798 Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#issuecomment-1251283378
please rebase/squash these commits before merge but other than that - 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] spmallette commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970966822
##########
gremlin-test/features/map/Edge.feature:
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@StepClassMap @StepE
+Feature: Step - E()
Review Comment:
I assume it should work without any trouble but I think it's worth adding some tests for where your new mid-traversal `E()` is taking some arguments (including `null`).
--
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] vkagamlyk commented on pull request #1808: TINKERPOP-2798 Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#issuecomment-1258632869
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] spmallette commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r972391096
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
when you merge from 3.5-dev to 3.6-dev and 3.6-dev to master, yes, you will need to add some follow-on work there to cover the new steps that we've added like `mergeV/E()`. i guess this isn't a breaking change though so you should be able to just issue a follow-on PR without having to worry about breaking the build in either of the downstream branches.
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970742051
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
@vkagamlyk would you mind taking the extra step to fix it on this PR? or an immediate follow-up PR? i'd hate for us to forget this change when we have a chance to instituted it on 3.7.x and let this release with the underscore.
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969983197
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java:
##########
@@ -331,6 +331,21 @@ public default GraphTraversal<S, Vertex> V(final Object... vertexIdsOrElements)
return this.asAdmin().addStep(new GraphStep<>(this.asAdmin(), Vertex.class, false, ids));
}
+ /**
+ * A {@code E} step is usually used to start a traversal but it may also be used mid-traversal.
+ *
+ * @param edgeIdsOrElements vertices to inject into the traversal
Review Comment:
"vertices" should be "edges" in the javadoc there.
--
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 diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970537109
##########
gremlin-dotnet/src/Gremlin.Net/Gremlin.Net.csproj:
##########
@@ -67,7 +67,7 @@ NOTE that versions suffixed with "-rc" are considered release candidates (i.e. p
<RepositoryUrl>https://github.com/apache/tinkerpop</RepositoryUrl>
<PublishRepositoryUrl>true</PublishRepositoryUrl>
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
- <EnablePackageValidation>true</EnablePackageValidation>
+ <EnablePackageValidation>false</EnablePackageValidation>
Review Comment:
Please revert this change. Package validation ensures that we don't accidentally add breaking changes without being aware of it.
In this case, we accept the breaking change of renaming the generic type parameter from `E` to `E_` of `GraphTraversal`. So, we can silence the warning for this change. You can do this by adding the following entry to the file `CompatibilitySuppressions.xml` in `gremlin-dotnet/src/Gremlin.Net`:
```xml
<!-- Renamed generic type parameter name in GraphTraversal -->
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Gremlin.Net.Process.Traversal.GraphTraversal`2</Target>
<Left>lib/netstandard2.0/Gremlin.Net.dll</Left>
<Right>lib/netstandard2.0/Gremlin.Net.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
```
behind the other two suppression entries there.
I realize though that this compatibility checking and how to deal with its warnings isn't easy to understand, especially for non-.NET developers who just want to modify Gremlin and therefore also have to make changes in .NET. So, I'm wondering whether we should keep the feature at all. Nevertheless, that shouldn't be part of this PR, so please add the suppression for now and reenable the validation here.
--
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] codecov-commenter commented on pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#issuecomment-1245839715
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1808?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1808](https://codecov.io/gh/apache/tinkerpop/pull/1808?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9e8f302) into [master](https://codecov.io/gh/apache/tinkerpop/commit/5b65d10393c3dd9466aa2a2b9677657f6e33b30a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b65d10) will **decrease** coverage by `5.09%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1808 +/- ##
============================================
- Coverage 69.20% 64.11% -5.10%
============================================
Files 875 23 -852
Lines 41744 3684 -38060
Branches 5622 0 -5622
============================================
- Hits 28890 2362 -26528
+ Misses 10876 1154 -9722
+ Partials 1978 168 -1810
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1808?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [gremlin-go/driver/anonymousTraversal.go](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvYW5vbnltb3VzVHJhdmVyc2FsLmdv) | `47.13% <100.00%> (+0.46%)` | :arrow_up: |
| [gremlin-go/driver/graphTraversal.go](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvZ3JhcGhUcmF2ZXJzYWwuZ28=) | `85.48% <100.00%> (+0.10%)` | :arrow_up: |
| [...in/language/grammar/DefaultGremlinBaseVisitor.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9sYW5ndWFnZS9ncmFtbWFyL0RlZmF1bHRHcmVtbGluQmFzZVZpc2l0b3IuamF2YQ==) | | |
| [...emlin/language/grammar/TraversalMethodVisitor.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9sYW5ndWFnZS9ncmFtbWFyL1RyYXZlcnNhbE1ldGhvZFZpc2l0b3IuamF2YQ==) | | |
| [...in/process/traversal/dsl/graph/GraphTraversal.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9kc2wvZ3JhcGgvR3JhcGhUcmF2ZXJzYWwuamF2YQ==) | | |
| [...kerpop/gremlin/process/traversal/dsl/graph/\_\_.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9kc2wvZ3JhcGgvX18uamF2YQ==) | | |
| [...ture/io/binary/types/OffsetDateTimeSerializer.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvaW8vYmluYXJ5L3R5cGVzL09mZnNldERhdGVUaW1lU2VyaWFsaXplci5qYXZh) | | |
| [...gremlin/structure/io/binary/GraphBinaryMapper.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvaW8vYmluYXJ5L0dyYXBoQmluYXJ5TWFwcGVyLmphdmE=) | | |
| [...aversal/traverser/LP\_O\_OB\_P\_S\_SE\_SL\_Traverser.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC90cmF2ZXJzZXIvTFBfT19PQl9QX1NfU0VfU0xfVHJhdmVyc2VyLmphdmE=) | | |
| [...mlin/driver/ser/GraphSONMessageSerializerV3d0.java](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1kcml2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3RpbmtlcnBvcC9ncmVtbGluL2RyaXZlci9zZXIvR3JhcGhTT05NZXNzYWdlU2VyaWFsaXplclYzZDAuamF2YQ==) | | |
| ... and [844 more](https://codecov.io/gh/apache/tinkerpop/pull/1808/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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 #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#issuecomment-1245840038
needs a CHANGELOG entry and Upgrade Documentation, but looking pretty good. thanks!
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969982023
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -1236,8 +1236,8 @@ g.V()
[[e-step]]
=== E Step
-The `E()`-step is meant to read edges from the graph and is usually used to start a `GraphTraversal`. Unlike the related
-<<v-step,V()>> it cannot be used mid-traversal.
+The `E()`-step is meant to read edges from the graph and is usually used to start a `GraphTraversal`, but can also
+be used mid-traversal.
Review Comment:
please add an example of mid-traversal `E()` here
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r972359040
##########
CHANGELOG.asciidoc:
##########
@@ -30,6 +30,10 @@ This release also includes changes from <<release-3-6-XXX, 3.6.XXX>>.
* Added warning on vertex property cardinality mismatch when reading GraphML.
* Bumped to `ws` 8.x for `gremlin-javascript`.
+==== Improvements
+
+* TINKERPOP-2798 Add support for mid-traversal E()
Review Comment:
updated in https://github.com/apache/tinkerpop/pull/1808/commits/41240b356c3cde71f673952d27ef061983402c9d
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970086286
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java:
##########
@@ -331,6 +331,21 @@ public default GraphTraversal<S, Vertex> V(final Object... vertexIdsOrElements)
return this.asAdmin().addStep(new GraphStep<>(this.asAdmin(), Vertex.class, false, ids));
}
+ /**
+ * A {@code E} step is usually used to start a traversal but it may also be used mid-traversal.
+ *
+ * @param edgeIdsOrElements vertices to inject into the traversal
Review Comment:
fixed in https://github.com/apache/tinkerpop/pull/1808/commits/13501c50233d1692d2cd3dd8185e61e9050f8fe3
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r971410106
##########
gremlin-test/features/map/Edge.feature:
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+@StepClassMap @StepE
+Feature: Step - E()
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1808/commits/edd2160056fcd2816e565ee7da757479dc139502
--
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] vkagamlyk merged pull request #1808: TINKERPOP-2798 Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk merged PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969993381
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java:
##########
@@ -331,6 +331,21 @@ public default GraphTraversal<S, Vertex> V(final Object... vertexIdsOrElements)
return this.asAdmin().addStep(new GraphStep<>(this.asAdmin(), Vertex.class, false, ids));
}
+ /**
+ * A {@code E} step is usually used to start a traversal but it may also be used mid-traversal.
+ *
+ * @param edgeIdsOrElements vertices to inject into the traversal
Review Comment:
```suggestion
* @param edgeIdsOrElements edges to inject into the traversal
```
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r971150000
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
@FlorianHockmann is it better to rename TStart/TEnd in all affected classes or only fix `GraphTraversal<TStart, TEnd >` in this PR?
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969981368
##########
docs/src/dev/future/index.asciidoc:
##########
@@ -49,7 +49,6 @@ line of 22H2.
* Add support for traversals as parameters for `V()`, `is()`, and `has()` (includes `Traversal` arguments to `P`)
* Geospatial support for TinkerPop (link:++https://lists.apache.org/list?dev@tinkerpop.apache.org:2021-7:DISCUSS%20geo-spatial++[DISCUSS Thread])
-* Add mid-traversal `E()` support
Review Comment:
could you please create a JIRA for this and reference it here rather than deleting the line? if folks want to know the status they could just look in on the ticket. i believe that's how it was being done prior to the 3.6.x release. it would be good to get the JIRA on this PR too so that it all links together nicely.
--
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 diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970729973
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
The C# [naming convention suggests](https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/generic-type-parameters#type-parameter-naming-guidelines) that names of generic type parameters like these should start with a _T_. We didn't follow that and instead just reused the names from Java _S_ and _E_, but I'm wondering if we should simply change these into _TS_ and _TE_ or even _TStart_ and _TEnd_ to be more descriptive and comply with the naming convention that most C# devs are used to.
This would then also solve this problem here.
If you don't have any objections, then I'll create a separate issue in Jira to change these names in a different PR. But this shouldn't block this PR here. So, I suggest that we keep it for now as you've did in this PR with `E_` and then we can later change that to `TE` / `TEnd` or whatever.
That's definitely better than changing the name of the step `E()` itself as that would lead to a lot more confusion in my opinion.
--
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 diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r972673601
##########
gremlin-dotnet/src/Gremlin.Net/Gremlin.Net.csproj:
##########
@@ -67,7 +67,7 @@ NOTE that versions suffixed with "-rc" are considered release candidates (i.e. p
<RepositoryUrl>https://github.com/apache/tinkerpop</RepositoryUrl>
<PublishRepositoryUrl>true</PublishRepositoryUrl>
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
- <EnablePackageValidation>true</EnablePackageValidation>
+ <EnablePackageValidation>false</EnablePackageValidation>
Review Comment:
You can generate the file automatically:
```bash
dotnet pack gremlin-dotnet/src/Gremlin.Net /p:GenerateCompatibilitySuppressionFile=true
```
or apparently also if you temporarily add this to the Gremlin.Net.csproj:
```xml
<GenerateCompatibilitySuppressionFile>true</GenerateCompatibilitySuppressionFile>
```
as described [here](https://docs.microsoft.com/en-us/dotnet/fundamentals/package-validation/overview#suppress-compatibility-errors).
This will remove the comments that we currently have in the file, but I guess those aren't that helpful any way so it shouldn't be a problem.
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r969992621
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
Class was defined as `public class GraphTraversal<S, E> : DefaultTraversal<S, E>`
[C# don't allow to have method with same name as type](https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0102)
Possible solutions:
1. rename `E` in class definition (internal effect only, I used it)
2. rename `E` function
--
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] vkagamlyk commented on pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#issuecomment-1245959394
> needs a CHANGELOG entry and Upgrade Documentation, but looking pretty good. thanks!
CHANGELOG updated in https://github.com/apache/tinkerpop/pull/1808/commits/13501c50233d1692d2cd3dd8185e61e9050f8fe3
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970001826
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
yeah, i was just making an observation. not much we can do there. personally, i don't mind the underscore but maybe @FlorianHockmann or others could offer a better suggestion?
--
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 a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970956888
##########
CHANGELOG.asciidoc:
##########
@@ -30,6 +30,10 @@ This release also includes changes from <<release-3-6-XXX, 3.6.XXX>>.
* Added warning on vertex property cardinality mismatch when reading GraphML.
* Bumped to `ws` 8.x for `gremlin-javascript`.
+==== Improvements
+
+* TINKERPOP-2798 Add support for mid-traversal E()
Review Comment:
The JIRA entries to CHANGELOG are added on release. You should add an entry above. That entry is typically meant to be more descriptive than the JIRA title or even more specific as you might do multiple entries per JIRA item.
--
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] vkagamlyk commented on a diff in pull request #1808: Mid-traversal E()
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r972340812
##########
gremlin-dotnet/src/Gremlin.Net/Gremlin.Net.csproj:
##########
@@ -67,7 +67,7 @@ NOTE that versions suffixed with "-rc" are considered release candidates (i.e. p
<RepositoryUrl>https://github.com/apache/tinkerpop</RepositoryUrl>
<PublishRepositoryUrl>true</PublishRepositoryUrl>
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
- <EnablePackageValidation>true</EnablePackageValidation>
+ <EnablePackageValidation>false</EnablePackageValidation>
Review Comment:
@FlorianHockmann Should I add all renamed classes and methods to `CompatibilitySuppressions.xml`?
How to make exclusion for method with generic param like `Gremlin.Net.Process.Traversal.GraphTraversalSource.Inject<S>(params S[])`?
--
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