Skip to content

TINKERPOP-1919 Merge classes P and TraversalPredicate in Gremlin.Net#816

Merged
asfgit merged 2 commits intotp32from
TINKERPOP-1919
Mar 13, 2018
Merged

TINKERPOP-1919 Merge classes P and TraversalPredicate in Gremlin.Net#816
asfgit merged 2 commits intotp32from
TINKERPOP-1919

Conversation

@FlorianHockmann
Copy link
Copy Markdown
Member

https://issues.apache.org/jira/browse/TINKERPOP-1919

This removes the TraversalPredicate class by including its functionality into P. That made it easier to enable our Gherkin runner to work with P.And() and P.Or() and having P as the type for step parameters is probably also easier to understand for users than TraversalPredicate.

I added a CHANGELOG entry as this is technically a breaking change as it removes a public class, but I think that it's unlikely that users interacted directly with TraversalPredicate instead of only via P.Something().And().

This affected 4 previously ignored scenarios. Unfortunately, 2 of those are still ignored because of TINKERPOP-1922.

VOTE +1

@jorgebay
Copy link
Copy Markdown
Contributor

Down to just 32 ignored!

VOTE +1

@spmallette
Copy link
Copy Markdown
Contributor

I think this is a good change though it doesn't exactly mimic the Java API right? there is no until(P) in java - it's until(Predicate), but I guess that doesn't really make sense in this GLV to support something like that because we can really only serialize values of P across to the server.

I agree that most folks will not have used this TraversalPredicate aspect of the API, so I agree that we can allow this breaking change to occur. However, I think that you should call more attention to it by adding something to the upgrade docs that describes the change and shows what the code used to look like and what it should look like now.

Does all of that make sense?

@FlorianHockmann
Copy link
Copy Markdown
Member Author

I think this is a good change though it doesn't exactly mimic the Java API right? there is no until(P) in java - it's until(Predicate), but I guess that doesn't really make sense in this GLV to support something like that because we can really only serialize values of P across to the server.

This will be changed with PR #815 that introduces new interfaces, one of which is IPredicate that we use in Gremlin.Net in places where Java uses Predicate. My idea was that users could write either predicates in the form of lambdas (PR #814) or implement their own predicates which would also require their own serialization. Geo-predicates or full-text search are an example where implementing custom predicates like this would make sense.

I'll add something for the upgrade docs. Although it will probably just concentrate on cases where users already implemented their own predicates as there should be no visible changes for users that only used our predicates. P.gt(1).and(P.lt(3)) can still be called exactly the same way. The only difference is that they now get back a P object instead of a TraversalPredicate.

@spmallette
Copy link
Copy Markdown
Contributor

Although it will probably just concentrate on cases where users already implemented their own predicates as there should be no visible changes for users that only used our predicates

exactly - VOTE +1

There is no good reason to keep those two classes separate anymore and
having P as the type for step parameters is probably easier to
understand for users than TraversalPredicate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants