looker-open-source / lookr

An R library for the Looker API (4.0)
MIT License
77 stars 29 forks source link

Change "View" in Looker to "Explore" & replace rJson with RJSONIO #7

Closed thekenz closed 8 years ago

thekenz commented 8 years ago

fixes #5

I chose to go with explore over base_view, because I thought that explore would be more clear for everyone, especially users of Looker that have never seen the base_view parameter, and have only dealt with explores.

Furthermore, I tested what LookR does where we alias the base view in the LookML using the view: parameter.

- explore: test
  view: order_items

Turns out that LookR will not return data in this instance:

my_inline_query <- run_inline_query(model = "kenny_test_postgres", 
                                    explore = "order_items", 
                                    fields = c("order_items.order_id", "order_items.id"))

but LookR will return data in this instance:

my_inline_query <- run_inline_query(model = "kenny_test_postgres", 
                                    explore = "test", 
                                    fields = c("order_items.order_id", "order_items.id"))

Thus, I changed the references in LookR to explore, for better clarity here.

githoov commented 8 years ago

@thekenz Look great! Maybe update the README example too?

thekenz commented 8 years ago

Fixes #6

@maxcorbin and I did some digging around here, and we found out that thefromJSON function in the rJson package that we've been using doesn't have a good way to parse null values. We found that the fromJSON package in both jsonlite (page 8) and JSONIO (page 4) were a lot more robust and provided a way to handle null values a lot better here.

Max found this document that compares rJson, rjsionio, and jsionlite. We ultimately went with jsonlite because it appeared to be more performant, and it handles matrices in a way that is similar to rJSON.

Tested it out using this script, and everything seems to check out.

library("devtools")
library("LookR")

# setting up the client and authenticating (id and secret are available on your Looker instance)
looker_setup(   id = "",
                secret = "",
                api_path = "https://learn.looker.com:19999/api/3.0"
)

# running a Look by its look_id
area_chart <- run_look(1597)

Also sorry for the bloated commit -- I was pushing to a fork I made of the repo, and totally forgot that additional commits would be piled onto this pull.

thekenz commented 8 years ago

Update: jsonlite actually rendered queries using run_inline_query in a strange way, so I went with RJSONIO instead. Went about update the dependencies, and the package imports from rJson to RJSONIO.

Tested both using this script:

# preliminaires #
library("devtools")
library("LookR")

# setting up the client and authenticating (id and secret are available on your Looker instance)
looker_setup(   id = "",
                secret = "",
                api_path = "https://learn.looker.com:19999/api/3.0"
)

# running a Look by its look_id
my_look <- run_look(1597)

# running an inline query by providing query components (note: model, view, and fields are required parameters)
my_inline_query <- run_inline_query(model = "ecommerce", 
                                    explore = "orders", 
                                    fields = c("orders.id"))

and the resulting dataframes seem to return very nicely.

maxcorbin commented 8 years ago

@thekenz Looks awesome, good call on rolling with RJSONIO. I'm wondering if the change from view to explore for the run_inline_query argument is a good idea, since the underlying JSON parameter is view.

I definitely agree that it is confusing, I actually just had a chat a few days ago where this came up. But it feels safer to keep R function arguments named identically to the JSON parameters they represent. Maybe we can talk to Engineering and have the view parameter renamed in the API before we rename it here?

githoov commented 8 years ago

Pulled and tested the changes, @thekenz. Looks great! Seems to fix issue #6.

I'm still not certain whether the migration to explore within the function is the best way to go—I just don't know. A more passive approach is to simply keep things as view and update the README with a comment, pointing to the fact that view here just means explore. That way we're consistent with the API documentation. If you and @maxcorbin come to an agreement on this, I'll merge the PR.

thekenz commented 8 years ago

Hey @githoov! I definitely agree with you, John, and @maxcorbin that keeping the view parameter inside the run_inline_query function is better in order to keep the functions in line with the API. I've gone ahead and changed that code back to view in run_inline_query, and added a flag in the README signifying that this parameter corresponds to the name of the explore, rather than the name of a view within an explore.

thekenz commented 8 years ago

I made the requisite changes here, and I think that this pull request is ready for merge!

thekenz commented 8 years ago

BOOM 💥 we are good to go!