Support named databases #33
Conversation
|
Hi @mitch-b, thank you for the contribution. |
Thanks, @katarinasupe, we'll use our fork until we can get it merged. |
|
The thought occurred to me to have an alternate constructor with a |
There was a problem hiding this comment.
Hi again @mitch-b, thanks for contributing 🦾 🚀 So I am more for making this part of the driver dict config, I have nothing against the ENV, we can have them and pack them into the dict depending on the type of env.
Also note for this PR, I am adding test to CI, After we merge CI test we will proceed with this.
| @@ -154,6 +154,15 @@ docker run --rm \ | |||
| mcp-memgraph:latest | |||
| ``` | |||
|
|
|||
| ### Docker environment variables | |||
There was a problem hiding this comment.
Now that we are adding them to the README, which we should have done previously, can you add them to a separate section called configuration, and then Envs as a subtitle, since these envs also work without Docker?
| @@ -13,6 +13,7 @@ def __init__( | |||
| url: str = None, | |||
| username: str = None, | |||
| password: str = None, | |||
| database: str = None, | |||
There was a problem hiding this comment.
I would expect this to live inside the driver configuration and not be visible inside the driver signature, as you have assumed.
Maybe we can group the ENVS that are driver specific, there is a lot of configuration on that side, so we do not continue to expand this signature, and instead just unpack the ENVS in MCP and pass the dictionary. What do you think?
This probably is a breaking change with the mid-signature adjustment to the Memgraph object from the toolkit.
If that needs to be adjusted, let me know. It made more sense where I placed it than after the dictionary, but maybe it belongs in the dictionary as a parameter? Looking for guidance.