Skip to content

TINKERPOP-2265 Deprecate remote Traversal.sideEffects#1162

Merged
spmallette merged 1 commit intotp33from
TINKERPOP-2265
Jul 23, 2019
Merged

TINKERPOP-2265 Deprecate remote Traversal.sideEffects#1162
spmallette merged 1 commit intotp33from
TINKERPOP-2265

Conversation

@spmallette
Copy link
Copy Markdown
Contributor

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

Users should not prefer use of cap() to directly return side-effects as part of standard traversal iteration. Expecting removal of the deprecated bits in 3.5.0.

VOTE +1

Copy link
Copy Markdown
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should impact Message Ops OPS_GATHER & OPS_KEYS in TraversalOpProcessor as well. Please mark them as deprecated or add an appropriate comment over the expected usage.

@spmallette
Copy link
Copy Markdown
Contributor Author

@divijvaidya thanks - i shouldn't have missed all those. Should look better now.

@divijvaidya
Copy link
Copy Markdown
Member

I am familiar with the server and the java driver code changes and they look good.

VOTE +1

Copy link
Copy Markdown
Contributor

@jorgebay jorgebay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a code example into how to migrate existing code.

I've added a few comments below.

Copy link
Copy Markdown
Contributor

@jorgebay jorgebay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VOTE +1

Copy link
Copy Markdown
Member

@FlorianHockmann FlorianHockmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added one comment, but the changes look good in general and I like that we can get rid of this functionality in the future :)

public static final String OPS_CLOSE = "close";

/**
* @deprecated As of release 3.3.8, not directly replaced in the protocol as side-effect retrieval after
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nitpick) I think these tokens should also be deprecated the same way in Gremlin.Net's Token class (and I don't know about gremlin-python and gremlin-javascript).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - added the obsolete annotation for .NET. javascript doesn't have side-effect functionality so nothing to deprecate there. i thought about python but it meant adding the deprecations library (i think) and i didnt' want to add a dependency (maybe we should...i dunno - perhaps that's a separate decision for newer versions of gremlin-python). how does everything look now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me in general, but it looks like you need to add a using directive in Tokens.cs which lets the build fail right now.

Users should not prefer use of cap() to directly return side-effects as part of standard traversal iteration.
@spmallette spmallette merged commit b355d87 into tp33 Jul 23, 2019
@spmallette spmallette deleted the TINKERPOP-2265 branch July 26, 2019 13:39
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