-
Notifications
You must be signed in to change notification settings - Fork 436
Auto Paging for Users in Groups #204
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
@@ -59,10 +73,6 @@ def remove_user(self, group_item, user_id): | |||
self._remove_user(group_item, user_id) | |||
try: | |||
users = group_item.users |
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 is actually unnecessary because every call is a new Pager now, but we could return the list from add_user
or we can just remove this from the call enitrely
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.
Can you show me an example of usage?
Usage: # get a list of users for each group
mapping = {}
groups = TSC.Pager(server.groups)
for group in groups:
server.groups.populate_users(group)
mapping[group.name] = group.users
# get users for one group, loop and do something
server.groups.populate_users(group)
my_users = group.users
for u in my_users:
do_the_thing(my_user)
# add/delete
server.groups.populate_users(group)
start = set(group.users)
server.groups.add_user([u1, u2, u3[)
end = server.groups.user
print(str(end - start)) |
…responses. Still need to add a len menthod to pager
@@ -57,28 +71,12 @@ def create(self, group_item): | |||
@api(version="2.0") | |||
def remove_user(self, group_item, user_id): | |||
self._remove_user(group_item, user_id) |
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.
At this point it may not even be necessary to have the _remove_user
function since we don't do anything special now
logger.info('Removed user (id: {0}) from group (ID: {1})'.format(user_id, group_item.id)) | ||
|
||
# Adds 1 user to 1 group | ||
@api(version="2.0") | ||
def add_user(self, group_item, user_id): | ||
new_user = self._add_user(group_item, user_id) |
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.
ditto
@RussTheAerialist Here's a cleaned up rev -- I've been testing manually for a bit and seems to work the way I think it should. I'd like to check this in and get @cmtoomey and @ugamarkj to check it out. If they like it, I can generalize it (I've got an idea to refactor it with passing callables all the way down) to other sub-models, whichever feels easier to maintain. |
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.
Small minor nitpick but otherwise good 🚀
tableauserverclient/server/pager.py
Outdated
if hasattr(endpoint, 'get'): | ||
# The simpliest case is to take an Endpoint and call its get | ||
self._endpoint = endpoint.get | ||
else: |
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.
Elif callable(...):
...
Else:
raise ...
(Sorry I'm on my phone, didn't bring a computer with me to California)
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.
Oh good call, a little cleaner!
Subsumes #147 because the git history got all weird.
I still need to make the tests work properly, but a manual test indicates this all works well.
/cc @cmtoomey who may want to try this out