-
Notifications
You must be signed in to change notification settings - Fork 7
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
Updates MongoDBGraphStore.__init__ signature #83
Updates MongoDBGraphStore.__init__ signature #83
Conversation
"db.create_collection.(coll_name, validator={'$jsonSchema': schema.entity_schema})" | ||
) | ||
else: | ||
client: MongoClient = MongoClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't assume that the collection doesn't already exist just because we're creating the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blink1073 This is inside a warning message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is: if the collection already exists, what is the behavior here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I see what happens now. There would be no validation added. I think we need to always attempt to add validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do attempt. It's in a try/except block. Let's discuss on slack if we want to delay release of a version for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not attempt for a collection that already exists, which may have been created by the user, no?
Link to a new passing patch after additional cases of adding validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Update signature of constructor to remove positional arguments, allows one of collection, or connection_string, database_name, and collection_name.
Link to a passing patch