GitHub user kushal256 opened a pull request:

https://github.com/apache/incubator-tinkerpop/pull/141

TINKERPOP3-886

Let me know if exceptions should be managed any differently, or if you see any issues.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kushal256/incubator-tinkerpop master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-tinkerpop/pull/141.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #141

commit 5c4e9660d7fdf3c6d3aa71dd3742a4f0d549cf01
Author: Kushal <kushal@foodinhood.com>
Date: 2015-11-08T00:44:43Z

TINKERPOP3-886: Allow any GraphReader/Writer to be persistence engine for TinkerGraph

commit 16faa852e350b4d06a1f600078b6604b568438a7
Author: Kushal <kushal@foodinhood.com>
Date: 2015-11-08T01:02:08Z

Merge remote-tracking branch 'upstream/master'

Github user dkuppitz commented on a diff in the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#discussion_r44267313

— Diff: docs/src/implementations.asciidoc —
@@ -630,7 +630,8 @@ TinkerGraph has several settings that can be provided on creation via `Configura
value is specified here, the the `gremlin.tinkergraph.graphFormat` should also be specified. If this value is not
included (default), then the graph will stay in-memory and not be loaded/persisted to disk.

Typo."the the" => "then the"

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047685

Thanks for working on this. I see you did something for reading a graph, but what about writing?

https://github.com/kushal256/incubator-tinkerpop/blob/master/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java#L296

Note that the docs needed to be updated:

https://github.com/apache/incubator-tinkerpop/blob/master/docs/src/implementations.asciidoc#configuration

Specifically, the configuration description for `gremlin.tinkergraph.graphFormat` needs something to denote this new feature.

Would be nice to see a test case as well. Perhaps you could stub a reader/writer and assert that the class gets created and the appropriate methods are called. wdyt?

I'm not sure how others feel, but I think we will wait to merge this until after release of 3.1.0-incubating (in code freeze for that now).

Github user dkuppitz commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047954

I agree, I'd also like to see that covered by one or two tests.

Github user kushal256 commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155581913

Thanks for your comments. Yes, there should be similar logic for writing
graph.

Also, I agree, there should be tests, but I'm not sure how to do so when
using reflection (Class.forName() and newInstance() for creating new
Io.Builder), with mockito's stubs. Any recommendations for how to do this?
On Nov 9, 2015 4:13 AM, "Daniel Kuppitz" <notifications@github.com> wrote:

> I agree, I'd also like to see that covered by one or two tests.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155047954>
> .
>

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155586988

Don't use mockito. Create your mock the old fashioned way and just create a concrete class in test that implements the interfaces you need.

Github user kushal256 commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-155627962

OK just checked in with an old fashioned mock.

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#discussion_r44941599

— Diff: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java —
@@ -23,19 +23,21 @@
import org.apache.tinkerpop.gremlin.TestHelper;
import org.apache.tinkerpop.gremlin.process.traversal.P;
import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.io.Io;
import org.apache.tinkerpop.gremlin.structure.io.IoCore;
+import org.apache.tinkerpop.gremlin.structure.io.IoRegistry;
import org.apache.tinkerpop.gremlin.structure.io.IoTest;
import org.junit.Ignore;
import org.junit.Test;

-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.File;
+import java.io.*;
— End diff –

not a big deal, but for future pull requests, note that for style purposes we don't use wildcard imports.

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-157080928

thanks for updating the PR. we're in the midst of releasing 3.1.0-incubating. we'll review this in more detail as soon as that is out the door.

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158053106

I think we're good to VOTE on this now. Ran `mvn clean install` and all tests pass. There's a few little odds/ends I'll cleanup after merge.

+1

Github user twilmes commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158146890

Looks good to me. Ran the tests and then played around with it in the shell a bit. Exception message is good on a bogus persistence engine and gryo and graphson work as expected.

+1

Github user okram commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158526811

I pulled the branch and `mvn clean install`'d. Everything passed. I also reviewed the code. I don't really know this area of the codebase but the code is in our style and Stephen agrees with the PR, so thats good enough for me.

VOTE +1

Github user dkuppitz commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158532860

I've been almost happy with the first version and now that something has been added to the docs:

VOTE: +1

Github user kushal256 commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158556607

Awesome! Apologies for the import with a wildcard, my Intellij did that
for me, I can clean that up, let me know if there are any additional
cleanup items, want to ensure everything meets the project's standards.

On Fri, Nov 20, 2015 at 1:43 PM, Daniel Kuppitz <notifications@github.com>
wrote:

> I've been almost happy with the first version and now that something has
> been added to the docs:
>
> VOTE: +1
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158532860>
> .
>

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158584247

@kushal256 not a problem - i figured your IDE was the culprit. if you'd like to amend your PR with the fix to the imports that will be fine.

The other thing we tend to do is mark all variables `final` (unless they are not - which really makes them jump out at you and makes you wonder why they are not when you see them in the code base). so for example you added:

```text
public static Io.Builder createIoBuilder(String graphFormat) throws ClassNotFoundException, IllegalAccessException, InstantiationException

```

`graphFormat`, `ioBuilder` and `ioBuilderClass` should be `final`. `IoCore` is also a "public" API class, so some javadoc on that method would be good.

And last but not least, if you want to really tie a bow on this, you would add an entry to the CHANGELOG for 3.1.1:

https://github.com/apache/incubator-tinkerpop/blob/master/CHANGELOG.asciidoc#tinkerpop-311-not-officially-released-yet

Please comment when you've updated the PR with those little changes and i'll merge after that.

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158980645

@kushal256 i see you made some changes - is this ready for me to merge now?

Github user kushal256 commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158981359

@spmallette, yes, I made the changes as per your recommendation, ready for
merge.

@kushal256 <https://github.com/kushal256> i see you made some changes - is
this ready for me to merge now?

—
Reply to this email directly or view it on GitHub
<https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-158980645>
.

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-tinkerpop/pull/141

Github user spmallette commented on the pull request:

https://github.com/apache/incubator-tinkerpop/pull/141#issuecomment-159111306

@kushal256 i've merged this one. thanks for working through all that. note that were some weird file permission changes that came with your PR. I reverted those:

https://github.com/apache/incubator-tinkerpop/commit/5f1a354e577b7c31f00469c91acae61a6144a85f

for some reason, all those files were made executable. ??? anyway, something to watch for your next PR. thanks again!