-
Notifications
You must be signed in to change notification settings - Fork 13
Introduce Driver.execute_query
#531
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
Conversation
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.
comprehensive suite of tests, nice!
|
||
if bookmark_manager is None: | ||
config["bookmarkManagerId"] = -1 | ||
elif bookmark_manager is not Ellipsis: |
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.
what's Ellipsis
? 🤔
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.
the ...
C: BEGIN {"[db]": "*"} | ||
S: SUCCESS {} | ||
{{ | ||
C: RUN "CREATE (p:Person{name:$name}) RETURN p.name AS name" { "name":"the person" } {} |
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.
can we make the query simpler? does it need to be that long? (applies to other places too)
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.
I made this way for making it easier to figure out what's going on.
self.assertEqual(eager_result.keys, ["n"]) | ||
self.assertEqual(eager_result.records, [ | ||
types.Record(values=[types.CypherInt(1)])]) | ||
self.assertIsNotNone(eager_result.summary) |
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.
you could check the query included the summary
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.
I checked some fields, the summary is quite extensive. Maybe we could adapt the test_summary
for testing the summary returned in the eager function.
Co-authored-by: Robsdedude <[email protected]>
Co-authored-by: Robsdedude <[email protected]>
225254d
to
c5ca626
Compare
tests/stub/driver_execute_query/scripts/transaction_chaining.script
Outdated
Show resolved
Hide resolved
tests/stub/driver_execute_query/scripts/transaction_chaining_custom_bmm.script
Outdated
Show resolved
Hide resolved
Co-Authored-By: Rouven Bauer <[email protected]>
…ustom_bmm.script Co-authored-by: Robsdedude <[email protected]>
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.
Fine with me as is 🚀
One of my points didn't get discussed
#531 (comment)
But why do we even omit them?
SessionRun
e.g., always transmitsparams
.
But I can make it work either way in the Python backend.
tests/stub/driver_execute_query/scripts/transaction_chaining_custom_bmm.script
Show resolved
Hide resolved
Co-authored-by: injectives <[email protected]>
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.
Working great on dotnet
No description provided.