Skip to content

Bookmarking #104

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

Merged
merged 4 commits into from
Dec 14, 2016
Merged

Bookmarking #104

merged 4 commits into from
Dec 14, 2016

Conversation

technige
Copy link
Contributor

@technige technige commented Dec 5, 2016

No description provided.

@technige technige changed the title Bookmarking [NOT READY FOR MERGE] Bookmarking Dec 5, 2016
@apcj apcj requested a review from lutovich December 8, 2016 09:26
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@nigelsmall review completed, couple small comments.

@@ -336,6 +338,8 @@ def run(self, statement, parameters=None, **kwparameters):
:return: Cypher result
:rtype: :class:`.StatementResult`
"""
self.last_bookmark = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do this initialization together with initialization in line 314?

Copy link
Contributor Author

@technige technige Dec 14, 2016

Choose a reason for hiding this comment

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

Ah, yes we do. This clears the bookmark before a run, it's not initialisation.

self.transaction = Transaction(self, on_close=clear_transaction)
return self.transaction

def commit_transaction(self):
self.run("COMMIT")
result = self.run("COMMIT")
self.connection.sync()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to test that we send commit and rollback messages eagerly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests added for that.

@@ -238,19 +243,21 @@ def __init__(self, sock, **config):
# Pick up the server certificate, if any
self.der_encoded_server_certificate = config.get("der_encoded_server_certificate")

def on_success(metadata):
self.server_version = metadata.get("server")
Copy link
Contributor

Choose a reason for hiding this comment

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

server_version field is initialized here but where is it read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just used in a test right now. It will conflict with #108 so I'll adjust it properly for that.

self.connection.in_use = False
self.connection = None

def begin_transaction(self):
def begin_transaction(self, bookmark=None):
""" Create a new :class:`.Transaction` within this session.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should docstring also say something about the bookmark parameter?

@technige technige merged commit a39c181 into 1.1 Dec 14, 2016
@technige technige deleted the 1.1-bookmarking branch December 14, 2016 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants