Skip to content

Conversation

@pm-osc
Copy link
Contributor

@pm-osc pm-osc commented Dec 25, 2023

JanusGraph-Python extends Apache TinkerPop™'s Gremlin-Python with support for JanusGraph-specific types. This version supports only GraphSON 3 serialization for RelationIdentifier and Text predicates. This version is compatible with JanusGraph 1.0.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@janusgraph-bot janusgraph-bot added the cla: external CLA is managed externally (by EasyCLA) label Dec 25, 2023
Copy link
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.

Thanks a lot for this great contribution and sorry that you had to wait so long for a first review!

I just reviewed the PR and also tried out the library a bit. I basically only found some nits which I directly commented inline. Overall the PR already looks really good :)

Just 2 other general comments:

  • It would be good to have GitHub Actions execute the tests automatically on every commit. So, some GH Actions config would be good to have. However, we can also do this in a follow-up contribution.
  • Codacy reports some issues on this PR. This is our static code analysis tool. I think most of them are false positives, like it's complaining about the use of asserts in the tests. But maybe you can check them and see if Codacy reported any real issues which you could then fix. For the false positives, we can probably create exemptions in Codacy.

Signed-off-by: pm-osc <pm2.osc@gmail.com>
Copy link
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.

This looks good to me. From my side, this is ready to be merged. I'd say we wait a few days in case someone else also wants to review this.

One thing to note for others: GH Actions doesn't run on this PR, although it contains config for GH Actions. However, it does work on the source branch of this PR in @pm-osc's fork: https://github.com/pm-osc/janusgraph-python/tree/jg1-initial-pr
I guess this is a security precaution from GitHub to not execute GH Actions in a PR which adds the action (?)
But the actions should work in our repo once this is merged. If not, then we should look into it.

@FlorianHockmann FlorianHockmann merged commit be75b82 into JanusGraph:master Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: external CLA is managed externally (by EasyCLA)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants