Cole-Greer commented on code in PR #3448:
URL: https://github.com/apache/tinkerpop/pull/3448#discussion_r3365926734
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/sideEffect/Tree.feature:
##########
@@ -206,36 +206,3 @@ Feature: Step - tree()
|--josh
|--peter
"""
-
- Scenario: g_V_out_treeXaX_selectXaX_countXlocalX
- Given the modern graph
- And the traversal of
- """
- g.V().out().tree("a").select("a").count(local)
Review Comment:
I guess these tests were broken as `tree().count(local)` was reliant on Tree
being a map to give the count. I'd ideally like to avoid removing these tests.
They weren't actually intended to test semantics of `tree().count(local)`, the
pair of tests was meant to verify that TreeSideEffectStep acts as a barrier
(processes all incoming traversers before producing a single result). Using
`count(local)` to introspect the Tree from within gremlin was simply a nice
convenience as `g.V().out().tree("a").select("a")` returns 6 references to the
same Tree object, which is difficult for the tests to handle.
What are your thoughts on adding in a new case to `CountLocalStep` to handle
`Tree`? It could simply call `rootNodes().size()` to retain the old semantics
of counting the number of roots, although the more intuitive semantics would be
if `tree().count(local)` mapped to the new `nodeCount()` method. I'm not sure
how others may feel about a minor semantic break such as this.
--
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]
