-
Notifications
You must be signed in to change notification settings - Fork 8
global: item revision endpoint #63
base: master
Are you sure you want to change the base?
global: item revision endpoint #63
Conversation
Things to consider:
|
c7e60fb
to
adbc09f
Compare
}, | ||
'crcrev': { | ||
'default_endpoint_prefix': True, | ||
'pid_type': 'crcrev', |
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.
Do you have a new pid_type
or can you use the 'crcitm'
?
695f0da
to
c5b368d
Compare
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 ok, have a couple of 'questions' there though.
output = json.loads( | ||
subprocess.check_output(cmd, shell=True).decode('utf-8') | ||
) | ||
assert output and 'hits' in output and 'hits' in output['hits'] |
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.
Shouldn't you check the amount of hits? (not sure if it's relevant though, maybe just the fact that the hits
key is there means that there have been some)
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.
Maybe even compare it to the expected dict like you do in the other test:
assert output == {
'hits': {
'hits': ...
}
}
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.
Ah, yes, you are correct :). But in order to make this happen I'd also have to change the examples.app:items
to create better fixtures. Since this PR grew quite a little already, maybe we can do this later.
Right now we just check that the endpoint is there.
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.
Sure, just make sure to open an issue on it just to not forget about it.
item_revision_search = ItemRevisionSearch() | ||
|
||
assert item_revision_search == item_revision_search[0] | ||
assert item_revision_search == item_revision_search.params() |
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.
What is this black magic?! Xd
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.
Our search classes subclass elasticsearch_dsl.Search
(at least invenio_search.RecordsSearch
does) and this class has the methods __getitem__
and params
that I had to mock for the desired functionality.
Those functions perform some changes (__getitem__
for example uses slicing for pagination) and then return themselves :)
LGTM |
|
||
from invenio_search import RecordsSearch | ||
from invenio_circulation.api import Item |
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.
@mvesper is the isort
enabled in /run-tests.sh
?
Normally, we use relative imports for current package, hence from .api import Item
. In anycase there should be an empty line before block of imports from current language. Just check it with isort invenio_circulation/search.py
.
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.
isort
is on and I changed it to relative import :)
c5b368d
to
269d44c
Compare
Looks good to me :) |
269d44c
to
12a5e36
Compare
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.
LGTM
--
Since it is already 2017, can you update copyrights in files you have touched?
12a5e36
to
69c8a37
Compare
@jirikuncar Changed the copyrights :) |
@mvesper you should just add 2017 and keep 2016. |
* Adds the endpoint configuration for `Item.find_by_holding`. Signed-off-by: Martin Vesper <[email protected]>
69c8a37
to
a2c2a18
Compare
@jirikuncar x) fixed |
Item.find_by_holding
.