-
Notifications
You must be signed in to change notification settings - Fork 814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TINKERPOP-2978 Implement all() and any() steps. #2228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just some documentation fixes
VOTE +1
...e/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
Outdated
Show resolved
Hide resolved
@GraphComputerVerificationInjectionNotSupported | ||
Scenario: g_injectXabc_cdeX_anyXeqXbcdXX | ||
Given the empty graph | ||
And using the parameter xx1 defined as "l[abc,cde]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason to use a parameter here? doesn't g.inject(['abc','cde'])...
work ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, parameters do work here. I originally wrote these as JUnit tests and when translating them over, I forgot that gremlin-lang allows for square brackets to create lists. Updated them to use this notation instead of parameters for some tests.
And using the parameter xx1 defined as "l[bcd,bcd]" | ||
And the traversal of | ||
""" | ||
g.inject(xx1).all(P.eq("bcd")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests should probably have some negative occurrences like:
g.inject(['bcd','bcd'],['bcd','xyz']).all(P.eq("bcd"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative occurrences is tested by a different scenario. This one was just meant to test that it worked properly with positive occurrences.
@GraphComputerVerificationInjectionNotSupported | ||
Scenario: g_injectXnull_abcX_allXTextP_startingWithXaXX | ||
Given the empty graph | ||
And using the parameter xx1 defined as "l[null,abc]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting the parsers for l[]
work like this. i guess it is treating "null" as null
and not as a String
and treating "abc" without the double quotes as a String
somehow?
...re/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStepTest.java
Show resolved
Hide resolved
...re/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStepTest.java
Show resolved
Hide resolved
* @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#all-step" target="_blank">Reference Documentation - All Step</a> | ||
* @since 3.0.0-incubating | ||
*/ | ||
public default <S2> GraphTraversal<S, E> all(final P<S2> predicate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is all
step return data type same as input data type? S==E?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are the same. Did you prefer that I can to just S without E?
Codecov Report
@@ Coverage Diff @@
## master #2228 +/- ##
============================================
+ Coverage 75.86% 76.21% +0.34%
- Complexity 12650 12690 +40
============================================
Files 1054 1031 -23
Lines 64123 60313 -3810
Branches 7083 7094 +11
============================================
- Hits 48645 45965 -2680
+ Misses 12831 11901 -930
+ Partials 2647 2447 -200
... and 29 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
37e60ed
to
a7bf934
Compare
Thanks Ken, LGTM VOTE +1 |
bbf9ca7
to
af47e8d
Compare
all() and any() of Proposal 3 are implemented in this PR. The rest will follow in a separate PRs.