-
Notifications
You must be signed in to change notification settings - Fork 448
#190 add update datasource connection #253
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
#190 add update datasource connection #253
Conversation
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.
All nits.
🚀
samples/update_connection.py
Outdated
|
|
||
|
|
||
| def main(): | ||
| parser = argparse.ArgumentParser(description='Get all of the refresh tasks available on a server') |
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.
The description is for the wrong sample (I bet a lot of our samples are, actually)
| resource = endpoint.get_by_id(args.resource_id) | ||
| endpoint.populate_connections(resource) | ||
| connections = filter(lambda x: x.id == args.connection_id, resource.connections) | ||
| assert(len(connections) == 1) |
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.
Do you have to list(filter Here for python3? I forget if filter_results objects have len
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.
Doh. Yes.
| return self._connection_type | ||
|
|
||
| def __repr__(self): | ||
| return "<ConnectionItem#{_id} embed={embed_password} type={_connection_type} username={username}>"\ |
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.
One day we can use f-strings. One day...
| # Update workbook_connection | ||
| def update_conn(self, workbook_item, connection_item): | ||
| @api(version="2.3") | ||
| def update_connection(self, workbook_item, connection_item): |
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.
Should probably set update_conn to update_connection for back compact; and throw a warning?
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.
Probably... I'll make the change.
|
I like it! We may want to let the guy who was working on adding multiple connections know (did that ever get done? I should pester Ben...) |
|
I tested the update_connection.py on my server version 2018.2, and it does not work. No error, but nothing is updated on server. Did this work for anyone? |
Clean up the API a bit because I didn't really click with it.