Skip to content
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

Add context manager to Txn and DgraphClientStub. #185

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

garylavayou
Copy link

Using Transaction with Context Manager

The Python context manager will automatically perform the "commit" action after all queries and mutations have been done and perform "discard" action to clean the transaction. When something goes wrong in the scope of context manager, "commit" will not be called, and the "discard" action will be called to drop any potential changes.

with client.begin(read_only=False, best_effort=False) as txn:
  # Do some queries or mutations here

or you can directly create a transaction from the Txn class.

with pydgraph.Txn(client, read_only=False, best_effort=False) as txn:
  # Do some queries or mutations here

client.begin() can only be used with "with-as" blocks, while pydgraph.Txn class can be directly called to instantiate a transaction object.

Use context manager to automatically clean resources

Use function call:

with pydgraph.client_stub(SERVER_ADDR) as stub:
    client = pydgraph.DgraphClient(stub)

Use class constructor:

with pydgraph.DgraphClientStub(SERVER_ADDR) as stub:
    client = pydgraph.DgraphClient(stub)

Note: client should be used inside the "with-as" block. The resources related to client will be automatically released outside the block and client is not usable anymore.

@CLAassistant
Copy link

CLAassistant commented Nov 29, 2022

CLA assistant check
All committers have signed the CLA.

@mangalaman93
Copy link
Contributor

@garylavayou Thank you for the PR. will you be able to rebase the change on the latest master?

@garylavayou
Copy link
Author

@garylavayou Thank you for the PR. will you be able to rebase the change on the latest master?

I have synced the forked repo with the latest master. Is that enough?

@mangalaman93
Copy link
Contributor

Could you also sign the CLA please before we review the code?

I have two comments/questions here:

  1. Will this API be useful to the users of pydgraph? I think so and I am getting more opinions here from my colleagues.
  2. We should add more tests trying to stress this code? Do you think you would be able to do that? I can also brainstorm with you or by myself and come up with a set of tests if you like.

@garylavayou
Copy link
Author

@mangalaman93 It would be better if you can add tests to the code, since I currently have no stable test environment. I am also looking forward that you can make the context manager more robust and extend the function with your opinions.

@mangalaman93
Copy link
Contributor

@garylavayou Thanks for responding back. I am busy with a few other things, I highly doubt if I could spend time writing tests right now. But I will keep this in mind. If you do get more time and opportunity to make changes, please go ahead and keep me posted. Appreciate your time and responding back.

Copy link

This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open.

@github-actions github-actions bot added the Stale label Jul 11, 2024
@garylavayou
Copy link
Author

This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open.

keep it open please!

@github-actions github-actions bot removed the Stale label Nov 12, 2024
Copy link

This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open.

@github-actions github-actions bot added the Stale label Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants