Skip to content

Add support for Memgraph #513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

katarinasupe
Copy link

@katarinasupe katarinasupe commented Jul 28, 2023

I added support for the Memgraph database. First, I added README to the additional databases folder to show that connecting to Memgraph is achievable via the existing Neo4j setup. But, since it's not intuitive for the users to set a username and password for the neo4j configuration setting, I created MemgraphSection in the configuration and exposed username, password, auth and database configuration flags. I added database section later because Memgraph is releasing a multitenancy feature in the newest version this week.

I also updated the README in the root folder accordingly.

Since I added Memgraph as an additional database, I also decided to contribute with a notebook. I am aware of the process of creating an issue first, but I hope you still will be okay with me adding this.

I would like to add that I missed local testing instructions to be sure all is alright, but I did test the connection to Memgraph and run queries from the Supply Chain Analysis notebook.

Let me know what I need to do else in order to get this PR merged and I will be more than happy to contribute more. I also noticed that my black formatter made some changes, but I did not read which formatter you use anywhere.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@triggan
Copy link
Contributor

triggan commented Aug 15, 2023

Thank you so much for submitting this! This is great!

We're in the process of reviewing everything that was submitted. There seems to be quite a few formatting changes, many of which are not related to the functional changes being requested. It would be helpful if you could resubmit this with only the functional changes and then maybe we can address the requested formatting changes in a different PR. With all of the formatting changes, it is requiring a lot of extra time to review everything that is here.

Copy link
Contributor

@krlawrence krlawrence left a comment

Choose a reason for hiding this comment

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

As mentioned - there are so many formatting changes it's really hard to spot the actual, substantive, changes. We're happy to also take PRs that improve formatting but so many changes such as a single quote being replaced by a double quote make it very hard to read the extremely large diffs that this has generated. Please back out the formatting changes into a separate PR. Thanks again for this submission.

@katarinasupe
Copy link
Author

Hi @krlawrence and @triggan, and thank you for taking a look at this PR. Sorry for including the formatting changes, I had my Black formatter turned on and I totally missed it. I will create a separate PR without formatting changes.

@katarinasupe
Copy link
Author

Hi @krlawrence and @triggan :) I added a new PR without formatting changes, so I guess this PR can be closed. Thanks again for checking it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Resolved
Development

Successfully merging this pull request may close these issues.

3 participants