Implement view filters on the populate* request options #260
Conversation
| @@ -105,7 +105,7 @@ def csv_fetcher(): | |||
| def _get_view_csv(self, view_item, req_options): | |||
| url = "{0}/{1}/data".format(self.baseurl, view_item.id) | |||
|
|
|||
| with closing(self.get_request(url, parameters={"stream": True})) as server_response: | |||
There was a problem hiding this comment.
I don't think this is right, the 'stream=true' is an option for the requests library, not something that gets included in the url string, which is what apply_query_parameters does.
| @@ -105,7 +105,7 @@ def csv_fetcher(): | |||
| def _get_view_csv(self, view_item, req_options): | |||
| url = "{0}/{1}/data".format(self.baseurl, view_item.id) | |||
|
|
|||
| with closing(self.get_request(url, parameters={"stream": True})) as server_response: | |||
| with closing(self.get_request(url, req_options, {"stream": True})) as server_response: | |||
There was a problem hiding this comment.
Thanks!
This might already by correct, but since I'm not 100% sure the order of function parameters as they get passed to the underlying requests.get, can you make the {"stream":True} a named parameter ( parameters=...)
Ugh, naming a parameter parameters is confusing. Thanks Requests....
| @@ -62,24 +62,49 @@ def apply_query_params(self, url): | |||
| return "{0}?{1}".format(url, '&'.join(params)) | |||
|
|
|||
|
|
|||
| class ImageRequestOptions(RequestOptionsBase): | |||
| class _FilterOptionsBase(RequestOptionsBase): | |||
There was a problem hiding this comment.
Seems like a reasonable refactor.
Can you give this class a docstring just so at a glance we can tell what it's used for vs other optionsbases?
| class ImageRequestOptions(RequestOptionsBase): | ||
| class _FilterOptionsBase(RequestOptionsBase): | ||
| def __init__(self): | ||
| self.view_filters = [] |
There was a problem hiding this comment.
Calling it view filters and then using it in CSV options is a little confusing to me -- can you tell me your thinking on this vs just 'filters'?
I'm not opposed just want to understand.
There was a problem hiding this comment.
We already have filters which is for filtering responses to the "get" calls. View Filters are different because they are filtering the view. A CSV is still a view of the data...
There was a problem hiding this comment.
These are filtering literal Views (Vizzes?) Ok that makes sense.
| return "{0}?{1}".format(url, '&'.join(params)) | ||
|
|
||
|
|
||
| class ImageRequestOptions(_FilterOptionsBase): | ||
| # if 'high' isn't specified, the REST API endpoint returns an image with standard resolution | ||
| class Resolution: | ||
| High = 'high' | ||
|
|
||
| def __init__(self, imageresolution=None): |
There was a problem hiding this comment.
Side note: I thought I made this so it was image_resolution... maybe I never checked that fix in?
I might make a PR after this merges
| def apply_query_params(self, url): | ||
| raise NotImplementedError() | ||
|
|
||
| def vf(self, name, value): |
There was a problem hiding this comment.
There's no code testing this or sampling this yet, but I'm guessing this makes it:
opts = ImageRequestOptions()
opts.vf('a', 1).vf('b', 2) etc?
Baby-builder pattern!
Did you still want to refactor this all to eventually just be builder-y and call str on those? (referenced in #212)
There was a problem hiding this comment.
Yes, eventually, we will refactor to a builder pattern!
|
Hi @RussTheAerialist Any ETA of this getting merged to |
|
Thank you for adding view filters - I needed it for requesting view images. Looking forward to seeing it merged into the master branch. |
|
Ditto, this is important for me as well! |
|
We released v0.7 on Wednesday which contains these changes. |
|
You rock, @RussTheAerialist! Thanks! |
This is fallout from a bug I was fixing elsewhere, but we should have them.