Implement Pager for auto-paging requests #90

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.

Sign up for GitHub

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

Jump to bottom

Merged

t8y8 merged 7 commits into tableau:development from t8y8:85-feature-pager

Oct 31, 2016

Collaborator

t8y8 commented Oct 31, 2016 •

edited

Loading

Here's a first draft based on @RussTheAerialist's code in pagination_sample.py.

Currently it:

Takes an endpoint as its argument instead of a get function, this is because these calls only apply to get anyway, so let's keep it clean.
Takes an options object and seeds the filters/sorts with it, as well as the initial page size
Can be used as an iterator

I would like it to:

Not do everything in the __call__ method. I think we can move that to a helper or to __init__
- We would make the first page call on instantiation, which populates some properties that could be useful (like __len__)
Since we know the total_avaliable we might be able to implement __len__. This gives users the ability to check the length to see if they really want to iterate over it or not.
I need to read more to see if I need to worry about __next__ or not.
Add tests

This script works (tested manually):

import tableauserverclient as TSC

auth_stuff = TSC.TableauAuth(username='USERNAME', password='PASSWORD')

server = TSC.Server("http://SERVER")
server.auth.sign_in(auth_stuff)

options = TSC.RequestOptions()
options.filter.add(TSC.Filter(TSC.RequestOptions.Field.Name,
                                         TSC.RequestOptions.Operator.Equals,
                                         "WORKBOOKNAME"))

# Call with a filter
wbs = TSC.server.Pager(server.workbooks, options)
print(len([i for i in wbs[)) # prints 1

# Get them all!
wbs = TSC.server.Pager(server.workbooks)
print(len([i for i in wbs])) # prints 1086 on my test server, that's all of em!
server.auth.sign_out()

Open Questions:

Where should it live? I'm not sure Server is the right place
Thoughts on implementing __len__?
Thoughts on implementing the primary logic somewhere other than __call__? It might just be me who thinks it's weird.

/cc @RussTheAerialist @LGraber


          Initial Draft of Paging Getter

d4fe1c3

graysonarts suggested changes

View reviewed changes

Contributor

graysonarts left a comment

I don't have strong opinions on location. @LGraber might, but having it live in the server package makes some sense since it's specific to how the server is returning information.

Also, we should be in the habit of including new tests for new functionality. I know I've been a bit lax on that recently myself, but we need to strive to be better than our previous selves :)

tableauserverclient/server/server.py Outdated

    
                  """

                  def __init__(self, fetcher, opts=None):

                      self._fetcher = fetcher.get

Contributor

graysonarts Oct 31, 2016

probably name the argument that is passed in endpoint

Collaborator Author

t8y8 Oct 31, 2016

Done.

tableauserverclient/server/server.py Outdated

    
                      self._fetcher = fetcher.get

                      self._options = opts

                  def __call__(self):

Contributor

graysonarts Oct 31, 2016

yeah this is not ideal. It should probably just live in __iter__ and we get rid of __call__

tableauserverclient/server/server.py Outdated

    
                      self._options = opts

                  def __call__(self):

                      current_item_list, last_pagination_item = self._fetcher(self._options)

Contributor

graysonarts Oct 31, 2016

this won't work if they set a page number in the options because count will never be greater than total available until we hit the end and throw an error.

We should check that a page number hasn't been set in the options, and if it has output a warning or throw an exception.

Collaborator Author

t8y8 Oct 31, 2016 •

edited

Loading

There's always a page number (it's defaulted to 1).

Should the logic be:

On first iteration, run the generator
Store some state that says "we're working on it now"
Then just exhaust the generator starting from that point, then they could, if they want, start on page 3

I think that can happen if:

__iter__ does the first call
__next__ does the remaining calls

LGraber reviewed

View reviewed changes

tableauserverclient/server/server.py Outdated

    
              class Pager(object):

                  """ This class returns a generator that will iterate over all of the results.

                  server is the server object that will be used when calling the callback.  It will be passed

Contributor

LGraber Oct 31, 2016

This comment does not appear to be relevant anymore

Contributor

LGraber Oct 31, 2016

why doesn't it work if we simply change how we initialize count?

tableauserverclient/server/server.py Outdated

    
                          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)

Contributor

LGraber Oct 31, 2016

This needs a bit more protection. Technically _load_next_page can return an empty list ... even after we fix our counting issue. This could happen if someone deletes an item while we are iterating. We should make sure the list is not empty before popping.

Contributor

LGraber commented Oct 31, 2016

Oh yeah ... why did you put this in the server.py file and not its own file?


          Address feedback on PR, add tests, and clean up some comments.

e2fbec6

t8y8 commented

View reviewed changes

test/test_pager.py Outdated

    
                          workbooks = TSC.Pager(self.server.workbooks, opts)

                          self.assertTrue(len(list(workbooks)) == 3)

                          # Starting on 3 with pagesize of 1 should get the last item

Collaborator Author

t8y8 Oct 31, 2016 •

edited

Loading

That's just a duplicate section, I'll remove

t8y8 added 2 commits

October 31, 2016 13:08


          Fix tests, pep8

e59860e


          Wow, Monday

6d15d32

Collaborator Author

t8y8 commented Oct 31, 2016 •

edited

Loading

Alright! Addressed feedback (in person, and on here) tests added, and tested manually against an internal Server with this code:

import tableauserverclient as TSC


auth_stuff = TSC.TableauAuth(username='USER', password='password')

server = TSC.Server("http://server")

server.auth.sign_in(auth_stuff)

print(server.auth_token)

options = TSC.RequestOptions(pagenumber=2, pagesize=300)

wbs = TSC.Pager(server.workbooks, options)
print("GOT WBS")
print(len({i.id for i in wbs}))
server.auth.sign_out()

LGraber reviewed

View reviewed changes

test/test_pager.py Outdated

    
                          self.assertTrue(len(list(workbooks)) == 3)

                          # Let's check that workbook items aren't duplicates

                          workbooks = TSC.Pager(self.server.workbooks)

Contributor

LGraber Oct 31, 2016

why did you need to call this again?

Contributor

graysonarts Oct 31, 2016

generators are one time use. Once you iterate over them, you cannot go back. He could have stored the output of list(workbooks) and then he wouldn't have had to call it twice.

Contributor

LGraber Oct 31, 2016

Technically he constructed a list in the line above ... he just didn't capture it. I don't care that much, though, as this is not actually hitting a server.

test/test_pager.py

    
                          # Register Pager with some pages

                          m.get(self.baseurl + "?pageNumber=1&pageSize=1", text=page_1)

                          m.get(self.baseurl + "?pageNumber=2&pageSize=1", text=page_2)

                          m.get(self.baseurl + "?pageNumber=3&pageSize=1", text=page_3)

Contributor

LGraber Oct 31, 2016

why are you registering these if we are not supposed to call them?

Contributor

graysonarts Oct 31, 2016

This is part of the requests-mock syntax. You register a url that will be called via requests, and the result you'd like returned when this is called (text= arguments), and then requests-mock does magic and returns the text when you make a requests call of the appropriate method to the appropriate url.

Contributor

LGraber Oct 31, 2016

Hahahaha. :) I know what it does. My comment was that we don't call these urls in this test so why do we need to register them. :)

Contributor

LGraber Oct 31, 2016

My bad ... it does get called (except maybe the first one). OOps

test/test_pager.py Outdated

    
                          page_3 = f.read().decode('utf-8')

                      with requests_mock.mock() as m:

                          # Register Pager with default request options

                          m.get(self.baseurl, text=page_1)

Contributor

LGraber Oct 31, 2016

Why are you registering this if the test is not supposed to call it?

Contributor

LGraber commented Oct 31, 2016

I am confused because the tests don't represent how we expect the users to use this class (ie you keep casting to list and doing len) and you don't have a sample of how to use it. Can you please add a sample

t8y8 added 2 commits

October 31, 2016 14:02


          Update Samples and feedback

080634b


          final comment tweaks

738e490

graysonarts approved these changes

View reviewed changes

Contributor

graysonarts left a comment

🚀

LGraber approved these changes

View reviewed changes

Contributor

LGraber left a comment

🚀


          final bit

55d102c

graysonarts approved these changes

View reviewed changes

Contributor

graysonarts left a comment

🚀 x2

test/test_pager.py

    
                          workbooks = TSC.Pager(self.server.workbooks)

                          self.assertTrue(len(list(workbooks)) == 3)

                          workbooks = list(TSC.Pager(self.server.workbooks))

                          self.assertTrue(len(workbooks) == 3)

Contributor

graysonarts Oct 31, 2016

nitpick

self.assertEquals(len(workbooks), 3)

t8y8 merged commit a6d0ede into tableau:development

t8y8 deleted the 85-feature-pager branch

October 31, 2016 21:27

t8y8 mentioned this pull request

Implement Pager per #72 feedback #85

Closed

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request


          Removing unused imports per linter report (tableau#90)

c056ed1

bryceglarsen pushed a commit to bryceglarsen/server-client-python that referenced this pull request


          Removing unused imports per linter report (tableau#90)

200c3f8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet