Feature/extend item crud operations#42
Feature/extend item crud operations#42mdwRepository wants to merge 7 commits intothe-library-code:mainfrom
Conversation
merge release 0.13
| 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! |
kshepherd
left a comment
kshepherd
left a comment
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) ?
|
hi @mdwRepository just wanted to check if this is still active work? Would you like help resolving conflicts / making some of the requested changes? |
4 participants
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: