Cole-Greer commented on code in PR #3244:
URL: https://github.com/apache/tinkerpop/pull/3244#discussion_r2441211868
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -623,8 +623,45 @@ gremlin> g.V().choose(__.values("age")).
==>peter
----
+*Removal of choose().option(Traversal, v)*
+
+The `choose().option(Traversal, v)` is rarely used in comparison to the other
overloads with constants, predicates and Pick
+tokens. It is also not the recommended approach for performance reasons. In
addition, the current implementation leads to
+confusion as it only evaluates if the traversal is productive, rather than
performing comparison on based on the traversal's
+output value. Fixing this behavior would require extensive changes, as such we
are removing this functionality from
+`choose()` in 3.8.0, with plans to reintroduce a properly implemented version
in the next major release.
Review Comment:
I think this would be better restructured to keep the focus on the confusing
nature of the old implementation. Something such as this:
```
The `choose().option(Traversal, v)` was relatively unused in comparison to
the other overloads with
constants, predicates and Pick tokens. The previous implementation often led
to confusion as it only
evaluated if the traversal was productive, rather than performing
comparisons based on the traversal's
output value. To eliminate this confusion, `Traversal`'s are no longer
permitted as an option token for
`choose()`. Any usages which are dependent on the Traversal for dynamic case
matching can be
rewritten using `union()`, with filters prepended to each child traversal.
```
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -623,8 +623,45 @@ gremlin> g.V().choose(__.values("age")).
==>peter
----
+*Removal of choose().option(Traversal, v)*
+
+The `choose().option(Traversal, v)` is rarely used in comparison to the other
overloads with constants, predicates and Pick
+tokens. It is also not the recommended approach for performance reasons. In
addition, the current implementation leads to
+confusion as it only evaluates if the traversal is productive, rather than
performing comparison on based on the traversal's
+output value. Fixing this behavior would require extensive changes, as such we
are removing this functionality from
+`choose()` in 3.8.0, with plans to reintroduce a properly implemented version
in the next major release.
+
+[source,text]
+----
+// 3.7.x
+gremlin> g.V().hasLabel("person").choose(values("name")).
+......1> option(is("marko"), values("age")).
+......2> option(none, values("name"))
+==>29
+==>vadas
+==>josh
+==>peter
+
+// 3.8.0 - an IllegalArgumentException will be thrown
+gremlin> g.V().hasLabel("person").choose(values("name")).
+......1> option(is("marko"), values("age")).
+......2> option(none, values("name"))
+Traversal is not allowed as a Pick token for choose().option()
+Type ':help' or ':h' for help.
+Display stack trace? [yN]n
+// use predicates in these cases
+gremlin> g.V().hasLabel("person").choose(values("name")).
+......1> option(P.eq("marko"), values("age")).
+......2> option(none, values("name"))
+==>29
+==>vadas
+==>josh
+==>peter
+----
Review Comment:
I'm not sure these are the best examples as I think that the simpler
`option("marko", values("age"))` case is generally preferable to either `is()`
or `P.eq()`. I think the most important case to cover is when the traversal is
used for a more dynamic filter which cannot be easily adapted to the simpler
predicate options. In that case, we should show users how they can use
something like union() to achieve a similar result to the old
`choose().option(Traversal, v)`
```
[source,text]
----
// 3.7.x
gremlin> g.V().hasLabel("person").choose(identity()).
......1> option(outE().count().is(P.gt(2)), values("age")).
......2> option(none, values("name"))
==>29
==>vadas
==>josh
==>peter
// 3.8.0 - an IllegalArgumentException will be thrown
gremlin> g.V().hasLabel("person").choose(identity()).
......1> option(outE().count().is(P.gt(2)), values("age")).
......2> option(none, values("name"))
Traversal is not allowed as a Pick token for choose().option()
Type ':help' or ':h' for help.
Display stack trace? [yN]n
// use union() in these cases
gremlin> g.V().hasLabel("person").union(
......1> where(outE().count().is(P.gt(2))).values("age"),
......2> __.not(where(outE().count().is(P.gt(2)))).values("name"))
==>29
==>vadas
==>josh
==>peter
----
```
--
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]
