search

JanusGraph / janusgraph-python

JanusGraph Python Gremlin Language Variant (GLV)
Other
22 stars 10 forks source link

Basic pr #37

Closed debasishdebs closed 4 years ago

debasishdebs commented 4 years ago

@FlorianHockmann : As per your comment, I have added a new PR,

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

  1. Added support for Serializing & De-serializing Relation Identifiers.
  2. Added support for querying based on Text Predicates like textFuzzy, textContains etc.
  3. Added Client to connect to JanusGraph which adds the JanusGraph serializers and deserializers so that is can be queried.
linux-foundation-easycla[bot] commented 4 years ago

CLA Check
One or more committers are not authorized under a signed CLA as indicated below. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

debasishdebs commented 4 years ago

@FlorianHockmann We will need to activate the CI/CD for this PR. Any idea how we can do that?

Also, I don't know why build fails on your machine. Is it something related to

File d:\projects\projects\janusgraph\janusgraph-python-new\target\reports\integrationtests\TextAttribute_tests.err: Fatal Python error: failed to get random numbers to initialize Python

I get that error on my Windows system as well, when I run integration-tests. Don't know why but I guess there is some sort of Limited support for docker library I'm using from Python. Because this errors seems to originate from there.

But when I comment out the integration tests, my build passes successfully. Please see the attached. Its on a clean environment as part of build so there is no pre installed pre requisites.

As for if you are concerned, if its bad we skip Integration tests, then no worries. You will just need to comment it locally to make build work on windows, but whenever the code is pushed to repo, it triggers my personal Travis CI job. You can check the build log of most Recent Commit and it passes successfully.

FlorianHockmann commented 4 years ago

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


Issues
======
+ Solved 44
- Added 4

Complexity increasing per file
==============================
- src/integrationtest/python/JanusGraphContainer.py  3
- src/integrationtest/python/DocTraversals_tests.py  1

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

Clones removed
==============
+ src/integrationtest/python/TextAttribute_tests.py  -6

See the complete overview on Codacy

debasishdebs commented 4 years ago

@FlorianHockmann : I resolved almost all of Codacy issues. I suspect out of the remaining 8 issues, 6 are False Positive.

Please have a look, and let me know if you agree, then we might need to add some rules to ignore these errors.

FlorianHockmann commented 4 years ago

This PR is target at the initial-pr branch which already contains the code from this PR plus more. So, this PR only deletes code which makes it hard to review. We would basically review the deletion of code. Wouldn't it make more sense to create a new empty branch and then target the PR on that branch?

We will need to activate the CI/CD for this PR. Any idea how we can do that?

I think that we need some code on master first that Travis can build to enable it for the repository. That's another reason why it makes sense to merge a small contribution first so we can already set up the CI pipeline.

Also, I don't know why build fails on your machine. Is it something related to

I'll test that again when I review this the next time (waiting for your response regarding the target branch above).

I resolved almost all of Codacy issues.

Sounds good, but Codacy only analyses the diff which is basically only code deletion in my PR. You'll have to look up the issues in Codacy's dashboard directly to see all or we need to target the PR to an empty branch.

debasishdebs commented 4 years ago

Thanks Florian for the response. Yes that does make sense. But i don't think i have the necessary permission to create a branch in official "Janusgraph-python" repository. Or do I? I think you being committer you should have that. In such case, can you create a branch, checked out from master (hence doesn't contain code). And let me know. I'll then raise PR from my branch of fork "basic-pr" targetted towards the new branch you created.

That being minimal code, we can quickly go through that and review. And merge towards master. Having CI builds working will be really great :-)

Also, So if we do create a new PR as discussed above targetted towards clone of master (empty one), the Codacy analysis then will run on the whole codebase as part of PR right?

On Fri, Jan 10, 2020, 5:41 PM Florian Hockmann notifications@github.com wrote:

This PR is target at the initial-pr branch which already contains the code from this PR plus more. So, this PR only deletes code which makes it hard to review. We would basically review the deletion of code. Wouldn't it make more sense to create a new empty branch and then target the PR on that branch?

We will need to activate the CI/CD for this PR. Any idea how we can do that?

I think that we need some code on master first that Travis can build to enable it for the repository. That's another reason why it makes sense to merge a small contribution first so we can already set up the CI pipeline.

Also, I don't know why build fails on your machine. Is it something related to

I'll test that again when I review this the next time (waiting for your response regarding the target branch above).

I resolved almost all of Codacy issues.

Sounds good, but Codacy only analyses the diff which is basically only code deletion in my PR. You'll have to look up the issues in Codacy's dashboard directly to see all or we need to target the PR to an empty branch.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JanusGraph/janusgraph-python/pull/37?email_source=notifications&email_token=AAHXREULX7GTTL7KN6U7JI3Q5BQWXA5CNFSM4KE2Q6EKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEITXMZY#issuecomment-573011559, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHXRESDFUS6MREFC6KUSY3Q5BQWXANCNFSM4KE2Q6EA .

FlorianHockmann commented 4 years ago

How about you just target master now? The original reasoning behind merging your PRs into a development branch was that we found issues during the review that we wanted to fix after merging the PR, but before having it in master. With a minimal PR it should be possible in general to get it into a state where it can directly be merged into master in my opinion. If you however think that it might be too much work to get it directly ready for master, then I can also create another development branch for this.

debasishdebs commented 4 years ago

@FlorianHockmann : Makes sense. Let me do one thing then. I'll close out this PR. Push my local branch basic-pr on my form with updates. Then create a PR with target as master. Does that makes sense to you?

Also, I've activate Codacy on my Forked branch (Which is basic-pr). Please check this link for detailed analysis.