PR Version 2 (Squashed commits)

[debasishdebs]

It involved Code Changes and the following are the features as part of this PR:

Added support for Serializing & De-serializing Relation Identifiers.
Added support for querying based on Text Predicates like textFuzzy, textContains etc.
Added Client to connect to JanusGraph which adds the JanusGraph serializers and deserializers so that is can be queried.
Added docs for the functionalities provided
Adds a installer script to build/install the library and supports Unix, Mac, Windows systems.

Fixing #19 #22 from previous PRs

Squashed commits into 4 commits (as few as possible with all possible authors) and planning to close #38

[linux-foundation-easycla]

• ✅ Debasish Kanhar (e41f426, 96b8166, b7c8fec, 602c22a, 61807ed, 13c9f68, 4ab895b, eec4ad3, b00d832, 4b50d40, be663a0, b99f66b, 15aa9b3)
• ✅ Misha Brukman (af4c1e7)

• ❌ Jason Plurad The commit (81d5bc0) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.
• ❌The commit (4dffa51) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve. For further assistance with EasyCLA, please submit a support request ticket.

[janusgraph-bot]

Please verify the committer name, email, and GitHub username association are all correct and match CLA records.

[debasishdebs]

@FlorianHockmann : I squashed all commits in my branck basiic-pr for PR #38 . I plan to close that as the number of commits was becoming huge.

I have also marked resolved a few of Codacy issues raised as those can be ignored while 3 more are remaiing which somehow got mixed while doing rebase. (But Build in Travis are working fine so functionality is good).

Can you please review this and let's move this forward?

Thanks

[FlorianHockmann]

Here is an overview of what got changed by this pull request:

Issues
======
- Added 9
           

Complexity increasing per file
==============================
- src/main/python/janusgraph_python/structure/io/GraphsonWriter.py  1
- src/unittest/python/structure/io/GraphsonReader_test.py  4
- src/integrationtest/python/TextAttribute_tests.py  2
- src/unittest/python/core/datatypes/RelationIdentifier_test.py  1
- src/main/python/janusgraph_python/serializer/RelationIdentifierSerializer.py  1
- src/main/python/janusgraph_python/core/attribute/Text.py  1
- src/main/python/janusgraph_python/serializer/RelationIdentifierDeserializer.py  1
- src/unittest/python/serialization/RelationIdentifierSerializer_test.py  1
- build.py  1
- src/main/python/janusgraph_python/driver/ClientBuilder.py  4
- src/unittest/python/structure/io/GraphsonWriter_test.py  4
- src/integrationtest/python/JanusGraphContainer.py  3
- src/main/python/janusgraph_python/structure/io/GraphsonReader.py  1
- src/main/python/janusgraph_python/core/datatypes/RelationIdentifier.py  2
- src/unittest/python/serialization/RelationIdentifierDeserializer_test.py  1
- src/integrationtest/python/DocTraversals_tests.py  1
         

Clones added
============
- src/integrationtest/python/TextAttribute_tests.py  1
- src/integrationtest/python/DocTraversals_tests.py  1
         

See the complete overview on Codacy

[FlorianHockmann]

Issue found: [list-item-spacing] Missing new line after list item

[FlorianHockmann]

Issue found: [no-consecutive-blank-lines] Remove 1 line after node

[FlorianHockmann]

Issue found: [list-item-spacing] Missing new line after list item

[FlorianHockmann]

I just wanted to review this again, but I cannot build it and I want to be sure that it's possible for everyone to build this locally and that CI is working properly before I review this further.

[FlorianHockmann]

(nitpick) I think this sentence together with the following example could simply be expressed by just writing the JanusGraph version numbers as 0.3.z in the table.

[debasishdebs]

Was added as example to help newbies understand how symver works, and not to get confused. Can be removed if needed anything is okay :-)

[FlorianHockmann]

OK, I personally don't think that users of the library need to understand semver and that seeing 0.1.z -> 0.3.z is already enough to understand the compatibility (like we also do it for JanusGraph itself where the matrix is more complicated), but we can also keep this as is if you prefer it that way. So, feel free to just mark this as resolved.

[FlorianHockmann]

We wanted to merge the PR first into an intermediate branch as it was a really big PR and we wanted to clean up some things after merging the PR. However this new PR is pretty minimalistic so I hope that we can get it in a state where we can release the library directly after merging it, or maybe with minimal work post merge. That's why I think that it's OK to target master here. If we however notice during the review that more work is necessary, then we can still change the target branch to an intermediate branch.

[FlorianHockmann]

The recent changes you made look good overall, but following PEP 8 is still open in my opinion.

[debasishdebs]

@FlorianHockmann : Pushed a commit where I changed the module names to follow PEP8 guidelines. Also changed a few of class variables and methods as well to follow PEP8.
I'm concerned about the janusgraph_python.core.attribute.text module. The method inside contains textPrefix etc. Do we change those to like text_prefix and so on as well?

I'll now go through each of your remaining comments and check which are resolved (non PEP8 based comments) and which aren't based on those if needed I'll push another commit.

[FlorianHockmann]

The method inside contains textPrefix etc. Do we change those to like text_prefix and so on as well?

I would have initially said yes, but then I checked gremlin-python again which seems to use the Gremlin-Java methods 1:1 like withRemote instead of with_remote. (I was a bit surprised by this as I remember the rough guideline for GLVs to stick to conventions of the specific language instead of just following Gremlin-Java, but looks like this wasn't followed for casing in gremlin-python.)
So, I'd say that we should try to stay consistent with gremlin-python and therefore stick to camelCase for methods that also exist in JanusGraph in Java.

[debasishdebs]

@FlorianHockmann : Pushed a commit. Well we will have for now only text and geo predicate module which will be reflecting camelCase from Java. When we will be extending this Lib, things will grow but let's document this if needed for reference what say?

The recent commit updates the DOCS based on the changes to class. Have removed redundant methods like toDict().

Also made few comments w.r.t structure.io.graphson and serializers modules.

Do have a look and let me know :-)

[mbrukman]

Same comment as above.

[debasishdebs]

There is dict definition here @mbrukman so don't understand exactly

[mbrukman]

Is this file only used here? If it's not used anywhere else, can we just inline it here?

[debasishdebs]

Well constants.sh is sourced only from build.sh but is used by build.py and docs/conf.py .

Reason I wanted to abstract the constants from build.sh is because after sometime build.sh becomes quite stable and we will be needed to only change contants.sh in future but I'm open to suggestions here

[mbrukman]

BTW, if you want to easily reformat your Python code to comply with PEP8, you can do that with autopep8; I've used it in several of my smaller Python projects with good results (but obviously, commit your local changes to a branch first before running it, so that you can easily revert them).

$ pip install autopep8
$ autopep8 --in-place file.py

See https://github.com/hhatto/autopep8 for more info.

[debasishdebs]

Pushed a fresh commit resolving few of your comments. Have marked those as resolved. Few more are open for discussion. Please do take a look @mbrukman

[FlorianHockmann]

We shall leave it to user to do customNamespace:typeID or janusgraph:MOCK or GraphSONUtil.formatType(prefix, typeID) outside of code is it?

Yes, users don't have to use a namespace at all if they don't want to. So I would just let them specify a type_id and they can use a namespace for that if they want, but they don't have to.

[FlorianHockmann]

Just made another pass through this and this time also reviewed the unit tests (integration tests and docs are still missing).

[debasishdebs]

I just verified we can't use json.dumps() here like you suggested in comment #39 (comment) @FlorianHockmann
I tried doing it as

mock_json = json.dumps({
            "@type": "janusgraph:MOCK",
            "@value": {"a": 1}
        })

and all unit tests started failing. I then verified, graphsonV3d0.py expects the input of toObject to be of dict and not json. Check https://github.com/apache/tinkerpop/blob/b73d2f3c0187dbdabc683260ace5cee17a4342aa/gremlin-python/src/main/python/gremlin_python/structure/io/graphsonV3d0.py#L124

[debasishdebs]

Looks like we have to use separators=(",", ":") always along with json.dumps() as has been followed as de-facto in gremlin-python tests for testing data types.
I've included those and now I'm able to make unit tests pass when I pass JSON String to methods like readObject and writeObject but I still don't understand why would we want to assert against String and not Dict ? Dict is more generic compared to string, and a single Dict can have multiple representations as we can see by making use of separators.

[debasishdebs]

@FlorianHockmann did you get time to have a look at updated commit?
Thanks

[FlorianHockmann]

Sorry that it took so long. I just reviewed the recent changes and they all look good to me. I have now also reviewed the integration tests and added comments there. (Docs are still missing.)
There are however still older comments open that aren't resolved like this one. (It's a bit hard to find comments like this now given the number of comments here. I only found it because I remembered that I commented on that.)

[debasishdebs]

@FlorianHockmann : Made another pass through existing comments. Resolved few open ones like type_id, symver, structure.io.graphson and other nitty ones.
Removed doc_traversal_tests integration test as it kinda seemed irrelevant and duplicate check to me.

Sorry took so long, have a look at fresh commit and do give your review :-) But this PR inspite of having so less files the conversation has become too big.

[debasishdebs]

@FlorianHockmann : Pushed a new commit. The current travis file doesn't catch build failures (maybe because it invokes multiple shell script and each shell script runs a command to build which starts a child process). So I've done changes to build script to catch build failures.

Also did changes to travis file so that we can start depreciating python 3.4 as its going to be soon depreciated as well. Added allow_job_failures.

Also plan to add support for building on osx and windows to travis later on.

[FlorianHockmann]

The recent changes look good to me. I'm just wondering about this one:

Added allow_job_failures.

Why allow job failures? If Python 3.4 doesn't work any more in a reliable way, then we shouldn't claim support for it and remove it completely.
If we however claim support for it, then the tests have to pass on Travis.

Also plan to add support for building on osx and windows to travis later on.

OK, but please don't add this to this PR. It's better to wait until we have merged this and released a first version of this lib in my opinion before we add anything more.

[FlorianHockmann]

This seems to contain some elements that aren't in the PR any more.

[FlorianHockmann]

adding Geoshape to dataset in JanusGraph sounds a bit strange. I would just write adding GeoShape data to JanusGraph

[FlorianHockmann]

JanusGraph Server

[FlorianHockmann]

(nitpick) Shouldn't we remove this comment? It seems to only have the purpose of explaining initially what this file is about, but that isn't needed any more now.

[FlorianHockmann]

Drivers? Why plural?

[FlorianHockmann]

Why aren't the properties also printed here? I think that would be helpful to understand what the predicate matched on.

[FlorianHockmann]

(nitpick) I think this example doesn't provide any additional value beyond the other two examples here so I would remove it.

[FlorianHockmann]

This isn't specific to TextContainsPrefix so I wouldn't explain it here. Otherwise, why isn't this also explained for the other TextContains_ predicates?

[FlorianHockmann]

TextPrefix wasn't even explained yet.

Why show the differences between these predicates here in the section of a specific predicate? If you want to show differences, then I would do that in the end after having introducing all the predicates on their own.

[FlorianHockmann]

I think there are too many examples below that don't add much value and make the section just longer. I would only provide a single and simple example per predicate and then maybe add a different example just add the end to show differences if that is really necessary (I think the description of the predicates already explains everything).

[FlorianHockmann]

@debasishdebs It has been over a year since the last update here and nearly 3 years since you've created your first PR (#2). Do you plan to get back to this and work through the review comments?
Otherwise, I could also give it a try. I would however probably use your code as the basis and then strip away as much as possible to get a first version that we can finally merge to master. Everything else can then still be added later with follow-up PRs. But I would really like to finally move forward here.

[debasishdebs]

@FlorianHockmann : Hi Florian, I moved from IBM to new company, and due to lot many factors have not been able to give time to this at all. Really sorry for that. Sad thing is, my current schedule will continue till mid of July and won't be able to work on this.

Post that I can pick that up, else I was thinking since these are pretty small issues, can we have a 2-3hours working session sometime after July mid and resolve all of your mentioned comments till then? What do you say?

If you feel mid July would be late, you can certainly pick it from here and we can close this out, but my availability before July looks really grim.

Thanks for understanding, and again sorry for inconvinience.

[FlorianHockmann]

Closing these 3 PRs as we have just merged another PR which contributed a first version for janusgraph-python.
@debasishdebs You are however of course more than welcome to still contribute to this library if you are still interested in that.