Improve test coverage and more intuitive Tags #191
Conversation
There was a problem hiding this comment.
Test coverage and __contains__ stuff generally looks good (didn't look that closely at the latter though). For adding support to query Samples by Tag, we should implement this using the API and do the filtering on the backend vs. the frontend. Example query in my comments inline below.
onecodex/models/sample.py
Outdated
| @@ -24,6 +24,11 @@ def where(cls, *filters, **keyword_filters): | |||
| public = keyword_filters.get('public', False) | |||
| instances_route = 'instances' if not public else 'instances_public' | |||
| limit = keyword_filters.get('limit', None if not public else 1000) | |||
| tag = keyword_filters.pop('tag', None) | |||
There was a problem hiding this comment.
So, we actually support this in the API and shouldn't need to get all samples and filter in memory. Here's an example public URL:
https://app.onecodex.com/api/v1/samples?where={"tags": {"$contains": {"$ref": "/api/v1/tags/dbcf5b98bca54a16"}}}
Consequently, I think the best way to implement this is to fetch the tag by name if we have a non-UUID (and maybe have some kind of is_uuid() function to skip this check) and then include it in the .where query directly.
Note we also implement a custom $containsany and $containsall selector in our backend that allows passing an array of tags and doing more complex intersects. We should look at the performance of that query code before leaning on it too heavily, but those may be useful too.
There was a problem hiding this comment.
OK, this was a lot simpler. Changed as requested.
onecodex/models/sample.py
Outdated
| if len(samples_to_keep) == limit: | ||
| break | ||
|
|
||
| samples = samples_to_keep |
There was a problem hiding this comment.
See comment above re: removing and simplifying this logic (plus shipping less data!).
| @@ -81,6 +89,13 @@ def test_resourcelist(ocx, api_data): | |||
| tags1.append(sample) | |||
| assert 'object of type' in str(e.value) | |||
|
|
|||
| if sys.version_info.major < 3: | |||
| with pytest.raises(AttributeError): | |||
There was a problem hiding this comment.
Just out of curiosity, why does this raise? Probably worth a comment for future us.
There was a problem hiding this comment.
Apparently lists in Python 2 do not have a clear() method
polyatail
added
in progress
polyatail
added
done
| new_tags.append(t) | ||
| else: | ||
| # is it a uuid? | ||
| if len(t) == 16: |
There was a problem hiding this comment.
Let's write an is_ocx_id helper function that does this plus and alphabet check. Something like:
assert set(x for x in name).issubset(string.hexdigits)
This PR addresses the following issues:
onecodex.Samples.where(tags='isolate')style syntax to return aSampleCollectionof samples matching a certain tag.