Implement view filters on the populate* request options#260
Implement view filters on the populate* request options#260graysonarts merged 6 commits intotableau:developmenttableau/server-client-python:developmentfrom graysonarts:add-view-filters-to-exportsCopy head branch name to clipboard
Conversation
| 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.
t8y8
left a comment
There was a problem hiding this comment.
Fit-It-Then-:rocket:
All minor
| 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....
|
|
||
|
|
||
| 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.
| 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.