Skip to content

Conversation

@martinmr
Copy link
Contributor

@martinmr martinmr commented Apr 16, 2019

Other clients do not have the query as part of the client interface,
only the transaction interface can do that. This change removes query
from the client interface so that it matches the rest of the clients.


This change is Reviewable

Other clients do not have the query as part of the client interface,
only the transaction interface can do that. This change removes query
from the client interface so that it matches the rest of the clients.
@martinmr martinmr requested a review from danielmai April 16, 2019 01:45
Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

:lgtm: Made a couple of comments about docstrings. Otherwise, looks good. This will be a breaking API change where users need to run Txn#query (which can be read_only or best_effort) instead of DgraphClient#query.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)


tests/test_essentials.py, line 32 at r1 (raw file):

        """Tests what happens when making a mutation on a txn after querying
        on the client."""

This docstring should be updated. The query's no longer done on the "client", which I presume means the DgraphClient object.

I'm glad this change makes the number of txns explicit.


tests/test_txn.py, line 239 at r1 (raw file):

        query = '{ me() {} }'

        # Using client.query helper method

Remove comment.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @danielmai)


tests/test_essentials.py, line 32 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…
        """Tests what happens when making a mutation on a txn after querying
        on the client."""

This docstring should be updated. The query's no longer done on the "client", which I presume means the DgraphClient object.

I'm glad this change makes the number of txns explicit.

Done.


tests/test_txn.py, line 239 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Remove comment.

Done.

@martinmr martinmr merged commit e14404d into master Apr 16, 2019
@martinmr martinmr deleted the martinmr/remove-query-from-client branch April 16, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants