Issue 112 - Support for the new image api endpoint by jz-huang · Pull Request #113 · tableau/server-client-python

jz-huang · 2016-12-14T02:33:33Z

added support for the new image api endpoint. Added unit tests and a sample. Added the new sample to docs.

Unit tests and pep8 run clean. Manually tested sample.

…the use of the new endpoint. added unit tests

jz-huang · 2016-12-14T02:38:30Z

Added a new ImageRequestOptions class because the "resolution" parameter doesn't really fit in with the current parameters in RequestOptions. Filter, sort, paging are all for endpoints that return a list of resources, doesn't really make sense to to add "resolution" to the group. And resolution is only parameter that's available for the image endpoint. With the new ImageRequestOptions, it would be easier to add more parameters for this endpoint in the future (i.e. filters within the viz).

t8y8 · 2016-12-14T07:26:18Z

Do we really need ABC and metaclass stuff? I think just a simple inherit is probably enough, it's a single method and not a complex class. It can also all probably rest in the same file -- splitting everything out into multiple files and enforcing the subclassing via metaclasses is very "java"

The rest of the code looks pretty solid, I'll do a detailed review tomorrow!

t8y8

One major objection, and then mostly small nits and questions

t8y8 · 2016-12-14T17:54:34Z

samples/get_view_image.py

+def main():
+
+    parser = argparse.ArgumentParser(description='Query View Image From Server')
+    parser.add_argument('--server', '-s', required=True, help='server address')


Please add site as well (I know not all samples have site yet, but you can avoid the rework later ;) )

@t8y8 i think you are saying "don't assume that everything is in the default site." yes?

Right, I noticed when messing around with 'livetests' that we assume the default site, we should take a site-id/content_url parameter for all of the samples (especially for Online folks)

sounds good, i'll add content_url as a parameter.

Thanks!

(Don't worry about the other samples for now, I might do that in a quick PR against development; and we don't want merge conflicts :) )

t8y8 · 2016-12-14T17:55:31Z

samples/get_view_image.py

+
+        # Step 2: Get all the projects on server, then look for the default one.
+        all_projects, pagination_item = server.projects.get()
+        default_project = next((project for project in all_projects if project.is_default()), None)


You can just use use TSC.Pager here.

(And this will fail if they have more than 100 projects and the default isn't in the first set of 100 returned)

t8y8 · 2016-12-14T17:57:04Z

samples/get_view_image.py

+            server.views.populate_image(view_item, image_req_option)
+
+            with open(args.filepath, "wb") as image_file:
+                image_file.write(view_item.image)


How large can this file get?

The files are usually a few hundred KB, probably gets to a couple of MB's at most.

Cool, we shouldn't have to worry about it then.

t8y8 · 2016-12-14T17:58:51Z

tableauserverclient/server/request_options_base.py

+import abc
+
+
+class RequestOptionsBase(object):


I think the abc and metaclass magic is unnecessary and doesn't really buy us anything but complexity to new contributors. It's kinda black arts level stuff :)

Ok, I wanted to take advantage of the builtin abstract class support. But it's kind of an overkill for this simple use case and like you said, probably makes the code more difficult to understand as well. will switch back to simple inheritance =)

Thank you!

Yeah, in this case it's not warranted (It's usually only done for large classes like a protocol in asyncio or twisted or something)

SIDE NOTE: I'm not a fan of the idea of having subclasses for all of our different url options/parameters, I feel like it'll just grow into a long list of "Which class do I need again", but in absence of a better idea I'll be quiet for now :)

I'm not a big fan of this either, but I didn't like adding "resolution" to RequestOptions. We don't have a way of dealing with this in the current design. I think the intention might be that once we add "fields" to RequestOptions that would cover all of the URL parameters. This endpoint is kind of an edge case. @LGraber probably have an opinion or two on this.

benlower · 2016-12-14T18:36:41Z

samples/get_view_image.py

@@ -0,0 +1,67 @@
+####
+# This script demonstrates how to use the Tableau Server Client
+# to query a high resolution image of a view from Tableau Server.


We're not really 'querying' an image are we? More like 'fetch' or 'download'?

Good catch, download is more appropriate, especially since we are saving the image to a specified filepath

benlower · 2016-12-14T18:39:47Z

samples/get_view_image.py

+    parser = argparse.ArgumentParser(description='Query View Image From Server')
+    parser.add_argument('--server', '-s', required=True, help='server address')
+    parser.add_argument('--username', '-u', required=True, help='username to sign into server')
+    parser.add_argument('--view-name', '-v', required=True, help='name of view')


can we expand the help text just a bit please? something like "name of the view we want an image of"

benlower · 2016-12-14T18:42:46Z

samples/get_view_image.py

+    # Step 1: Sign in to server.
+    tableau_auth = TSC.TableauAuth(args.username, password)
+    server = TSC.Server(args.server)
+    server.version = 2.5


@t8y8 @LGraber is the inclusion of 'server.version = x' the way we are indicating a min version for the sample? will the new auto-detect version help here or not? if we are going to keep server.version then let's add a comment that says this is the version in which this was introduced.

This will just fail if run against an old Server (actually, it will fail on sign in, not even on the new call).

My PR for detecting the version is outstanding, #100 -- waiting for approval to check it in.

But even then it wouldn't know which calls each version can and cannot make.

@RussTheAerialist had an idea to decorate the calls in the API with what version they use so we could throw a nice "NotSupported" exception or something, that work isn't yet started that I'm aware of (and is sorta dependent on my PR to get the version from Server)

benlower · 2016-12-14T18:44:15Z

samples/get_view_image.py

+        default_project = next((project for project in all_projects if project.is_default()), None)
+
+        # Step 3: If default project is found, download the image for the specified view name
+        if default_project is not None:


should we add optional ability to specify the project in the args or is that over-complicating the sample?

Good question.

All sites will have a default project, but exposing the ability to choose another might be nice (I think all the samples would want that though)

actually the views endpoint returns all the views on the site, so we don't need to specify the project. Copy and pasting problems... I'll remove the project related logic

benlower · 2016-12-14T18:46:03Z

samples/get_view_image.py

+            req_option = TSC.RequestOptions()
+            req_option.filter.add(TSC.Filter(TSC.RequestOptions.Field.Name,
+                                             TSC.RequestOptions.Operator.Equals, args.view_name))
+            all_views, pagination_item = server.views.get(req_option)


@t8y8 use pager here too? is our guidance to always use pager or only use it when you expect to have a lot of items (in this case we'd expect filter to reduce item count)?

Ha, good point.

But since he's not searching for one in particular, I don't think it matters much -- he just wants to get a view

benlower · 2016-12-14T18:47:51Z

samples/get_view_image.py

+            image_req_option = TSC.ImageRequestOptions(imageresolution=TSC.ImageRequestOptions.Resolution.High)
+            server.views.populate_image(view_item, image_req_option)
+
+            with open(args.filepath, "wb") as image_file:


what does "wb" do here? confusing to me b/c I assume it means workbook but this is an image.

"write binary" it's the standard in python code for opening a file in binary mode for writing

benlower · 2016-12-14T18:49:29Z

tableauserverclient/server/image_request_options.py

+
+class ImageRequestOptions(RequestOptionsBase):
+    class Resolution:
+        High = 'high'


is there only one option?

yes, it defaults to a standard resolution (~800x800), high doubles that.

ok. can you please add a comment here that says if you don't specify 'high' that the underlying REST API will default to standard?

benlower · 2016-12-14T18:52:39Z

test/test_view.py

+        with open(POPULATE_PREVIEW_IMAGE, 'rb') as f:
+            response = f.read()
+        with requests_mock.mock() as m:
+            m.get(self.baseurl + '/views/d79634e1-6063-4ec9-95ff-50acbf609ff5/image', content=response)


what server will this execute against? is this a valid view ID?

It's a mock, he's registering that URL so that when the API calls it it'll hit this fake endpoint and return the image

…d sample to download_view_image.py. Comments clean up

t8y8

🚀 I'll leave it up to you if you want to clean up my final nit

t8y8 · 2016-12-15T00:21:38Z

tableauserverclient/server/request_options.py

+    def __init__(self, imageresolution=None):
+        self.imageresolution = imageresolution
+
+    def image_resolution(self, imageresolution):


I called :)

This is unnecessary (and technically slower)

LGraber · 2016-12-15T16:32:24Z

samples/download_view_image.py

+        view_item = all_views[0]
+
+        # Step 3: Query the image endpoint and save the image to the specified location
+        image_req_option = TSC.ImageRequestOptions(imageresolution=TSC.ImageRequestOptions.Resolution.High)


I agree with Tyler. This is starting to feel heavy to code. The base RequestOptions felt more necessary because fields was more than a simple hash. As I watch this code being written in our samples, though, it is feeling heavier and heavier. It feels like the simple hash syntax might be more lightweight and we can still validate it. I am okay with you checking this in since it is in vnext but I think we should discuss this as I want users to be able to write minimal code that is easy to read and it is starting to feel like perhaps our use of enums and strongly typed objects is making the python harder and less ... pythonic. I am open to discussion.

I meant to say filter, not fields. Fields, also, was going to be interesting.

Thanks @LGraber , I opened #114 to track this idea -- I've been googling how some other libraries do it for inspiration (nothing struck me yet), but that'll make it so we don't lose the convo in this PR

jz-huang · 2016-12-15T18:38:17Z

I'm merging this in for now so Ben can WAM, let's keep the discussion going in #114.

jz-huang added 2 commits December 13, 2016 18:26

added support for new image api endpoint. added sample demonstrating …

8bac083

…the use of the new endpoint. added unit tests

added new sample to docs

2c2c8e0

jz-huang added the enhancement label Dec 14, 2016

jz-huang self-assigned this Dec 14, 2016

jz-huang requested review from LGraber and t8y8 December 14, 2016 02:33

t8y8 suggested changes Dec 14, 2016

View reviewed changes

benlower suggested changes Dec 14, 2016

View reviewed changes

jz-huang added 3 commits December 14, 2016 15:19

Response to code reviews. Put all request options into 1 file. rename…

053c585

…d sample to download_view_image.py. Comments clean up

update sample name in docs

bef42cf

pep8 compliance fix

0186de7

t8y8 approved these changes Dec 15, 2016

View reviewed changes

LGraber reviewed Dec 15, 2016

View reviewed changes

jz-huang merged commit 04398da into tableau:vnext Dec 15, 2016

Conversation

jz-huang commented Dec 14, 2016

Uh oh!

jz-huang commented Dec 14, 2016

Uh oh!

t8y8 commented Dec 14, 2016

Uh oh!

t8y8 left a comment

Choose a reason for hiding this comment

Uh oh!

t8y8 Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8y8 left a comment

Choose a reason for hiding this comment

t8y8 Dec 14, 2016 •

edited

Loading

t8y8 Dec 14, 2016 •

edited

Loading

t8y8 Dec 14, 2016 •

edited

Loading