-
Notifications
You must be signed in to change notification settings - Fork 445
Adding pagination sample #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
samples/pagination_sample.py
Outdated
| @@ -0,0 +1,79 @@ | |||
| #### | |||
| # This script demonstrates how to use pagination item that is returned as part | |||
| # of mayn of the .get() method calls. | |||
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.
mayn?
samples/pagination_sample.py
Outdated
|
|
||
| def main(): | ||
|
|
||
| parser = argparse.ArgumentParser(description='Explore workbook functions supported by the Server API.') |
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.
description needs updated for this sample
samples/pagination_sample.py
Outdated
| server = TSC.Server(args.server) | ||
|
|
||
| with server.auth.sign_in(tableau_auth): | ||
| generator = pagination_generator(server, lambda srv, opts: srv.workbooks.get(req_options=opts)) |
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.
Are there any other techniques that we might want to show here in addition to/instead of this?
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 do you mean? Do you have examples of what you are talking about?
t8y8
left a comment
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.
Approving with suggestions because we spoke about what's coming next anyway :)
| if len(current_item_list) == 0: | ||
| current_item_list, last_pagination_item = self._load_next_page(current_item_list, last_pagination_item) | ||
|
|
||
| yield current_item_list.pop(0) |
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.
This has to rebuild the list every time, only popping from the end is O(1).
I don't know if it matters that much here, if we decide it does some options:
- make current_item_list a deque
- pop from the end which would reverse the order from when w get the response but maybe it doesn't matter
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.
I think for the sample, I'm going to leave it as is, but replace this once I implement the larger effort.
| count = 0 | ||
|
|
||
| while count < last_pagination_item.total_available: | ||
| if len(current_item_list) == 0: |
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.
You can just do if not current_item_list but I think the equality so 0 might make this case a little clearer so not a strong recommendation.
| server = TSC.Server(args.server) | ||
|
|
||
| with server.auth.sign_in(tableau_auth): | ||
| generator = pagination_generator(server.workbooks.get) |
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.
Like we talked about ... this should take the options object so that we can support filters. I know I mentioned the optional additional arguments in case a get method takes more than just options but for now, you probably don't need to add this. Let's put the "paginator" in our server directory so that is it is a supported utility and then that will simplify our code. Would like Ben to comment on the name.
a78ae7c to
5f8ba95
Compare
|
Something is better than nothing. Would have liked to plumb through the options but I am good putting this in and then fixing it with the one we will officially support (as well as dealing with Tyler's comment) |
* Add description to the field object
Pull in the description if it exists on a `<column>` tag.
This change returns the entire `<desc>` tag including all subtags.
e.g.:
```
<desc>
<formatted-text>
<run>Total number of people in a country</run>
</formatted-text>
</desc>
```
* improving the test code to use 'in'
This sample has a generator class that makes it easy to iterate over all of the pages of one of the
.get()methods. We might want to consider making this class (or an updated version of it) part of the library.