-
Notifications
You must be signed in to change notification settings - Fork 16
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
[PLAT-4257] - return results from committing a transaction #157
[PLAT-4257] - return results from committing a transaction #157
Conversation
self.transaction = None | ||
|
||
return resp.json() |
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.
should we put this on a try block? I know we don't do this for a majority of the code, but I just ran into that thought. what if the request fails, and resp no longer has the .json()
method?
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 think if the request fails, a StardogException
should be raised iiuc. client.post
:
pystardog/stardog/http/client.py
Lines 52 to 53 in 8229d14
def post(self, path, **kwargs): | |
return self.__wrap(self.session.post(self.url + path, **kwargs)) |
returns the result from client.__wrap
which should check if the request failed and raise an exception:
pystardog/stardog/http/client.py
Line 67 in 8229d14
def __wrap(self, request): |
I think that's the order of execution but I might be misunderstanding/missing something.
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.
yes, I believe Noah is right. __wrap
should handle a failed request and then throwing the exception
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.
right, I forgot about it.
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.
looks good, just added one question
self.transaction = None | ||
|
||
return resp.json() |
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.
yes, I believe Noah is right. __wrap
should handle a failed request and then throwing the exception
Adds a response to the
commit
method. There is some helpful information in that response like the number of triples added/removed in that transaction.Example usage: