Feature/extend item crud operations #42
Conversation
merge release 0.13
| @@ -487,7 +487,7 @@ def search_objects( | |||
| size=20, | |||
| sort=None, | |||
| dso_type=None, | |||
| configuration='default', | |||
| configuration="default", | |||
There was a problem hiding this comment.
@kshepherd How much do we care about having a clean git blame?
I would strongly prefer at the very least that mere formatting changes like this be in their own commit. (I assume this is a black thing?)
| # Handle successful response | ||
| if response.status_code == 200: | ||
| # Parse the response JSON into an Item object | ||
| return self._construct_item(response.json()) |
There was a problem hiding this comment.
Oh: this is a breaking API change so we should bump to version 0.2 after merging this
|
Thanks for this! @kshepherd, who is the main maintainer of this library, is still on holiday right now, so it'll be another week or so yet before this gets approved, but I've noted one minor nit. I haven't fully reviewed and tested the new PATCH code yet but it looks great! |
There was a problem hiding this comment.
Thanks @mdwRepository for this PR, the operations are good, and I'm fine with the quote style changes.
I added a comment about _construct_item as it wasn't clear to me what we get out of it, versus just using the call it wraps, and asked whether we could expand it into a more generic construct function that would allow us to reuse it for many dso types.
| """ | ||
| try: | ||
| # Create an Item instance, using the API response data | ||
| return Item(api_resource=item_data) |
There was a problem hiding this comment.
Given all this does is one line, I'm wondering what the advantage of this function is... the auto exception handling?
My thinking when using the constructors like this is that it lets us be a bit more abstract, and do things like
dso_type = type(ds)
...
return dso_type(api_resource=parse_json(r))
Does that make sense? Do you think we need to just be more strict about handling each different DSO type? Or, if the error handling is the point, could we instead change this to _construct_dso(self, dso_data, dso_type) ?
The pull request mainly addresses the Add dedicated patch and delete item functions issue #41
Please note: The pull request also includes an enhanced get_item Method in DSpaceClient, which returns an item object and handles errors. (see issue #29)
The modified the get_item method: