Let gg2list return a figure object

[mkcor]

gg2list() returns a list of lists which is converted into the final JSON file by ggplotly().

It makes a lot more sense in the plotly framework to have gg2list() return a figure object, i.e., with dimensions data and layout. This way, users can apply it to a ggplot2 plot, say, gg:

fig <- gg2list(gg)
# Get/set layout attributes
fig$layout
# Get/set data attributes
fig$data
# And only then make the API call
py <- plotly()
py$plotly(fig$data, kwargs=list(layout=fig$layout))

It could be an opportunity to rewrite the plotly() method (as in py$plotly()), making things explicit and less 'flexible' (dot-dot-dot argument). But this is not as urgent so I have only adapted to the new figure return (7f4435c).

Tests won't pass as of commit 6b2a8e8, I'm going to make extensive use of sed to update our tests accordingly.

/cc @chriddyp @etpinard @cpsievert @tdhock

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/60482828

On TravisCI, commit d39157d was successfully merged with d9ecaf4 (master) to create 4e02b73. A visual testing table comparing d9ecaf4 with 4e02b73 can be found here:
http://ropensci.github.io/plotly-test-table/tables/4e02b73c95a648a454c05e450187600f0d896b5c/index.html

[cpsievert]

Assuming we want to go forward with the current design, these changes look good to me...

However, since this introduces some backwards-incompatible changes, I thought I'd mention again that having plotly() return a list of functions is confusing and unnecessary.

If plotly() was an S3 generic, it would solve our documentation problems and make the interface more intuitive for R users. If we go that route, it'd be convenient to attach a class (say 'plotly_figure') to the return value of gg2list(). That way, plotly() could take either a ggplot object or the return value of gg2list() as an input.

plotly <- function(p, username, key, ...)  UseMethod("plotly")

plotly.plotly_figure <- function(p, username, key, ...) {
  if (missing(username)) username <- Sys.getenv("PLOTLY_USER")
  if (missing(key)) key <- Sys.getenv("PLOTLY_KEY")
  # make call to API and return response
  make_call(p, username, key, ...)
}

plotly.default <- function(p, username, key, ...) {
  if (missing(username)) username <- Sys.getenv("PLOTLY_USER")
  if (missing(key)) key <- Sys.getenv("PLOTLY_KEY")
  # make call to API and return response
  make_call(gg2list(p), username, key, ...)
}

Of course, making this change would cause more serious backwards-incompatible changes, which will upset current users, but making the necessary changes shouldn't be that hard. @mkcor @chriddyp if you're willing to make these changes, please let me know, and I'll start a pull request.

[mkcor]

@cpsievert Thanks for your insight. I think this conversation belongs there #40 so I'll reply there about your re-design suggestions. I understand it's a related issue as far as we're introducing backwards incompatibility. I admit, I'm not very concerned with backwards incompatibility here (maybe I should care a lot, just by principle) because I doubt that many projects around the world rely on our gg2list() function... And even if they do... well, I know, I'm totally betraying my identity here ("Scientists want to explore. Engineers want to build." see http://www.software.ac.uk/blog/2015-02-06-scientific-coding-and-software-engineering-whats-difference)!

[mkcor]

@cldougl Once this is merged, gg2list() will return the expected figure object, so that there is no need for wrapping function gg2fig() (https://github.com/cldougl/plotly-shiny/blob/master/What_You_Need/plotlyGraphWidget.R#L18-L25).

[chriddyp]

I'm not too worried about backwards incompatibility since this is a private function anyway. This is a good step forward for usability in Shiny and some other customizability, so 👍 !

@cpsievert , @mkcor , @13bzhang , @cldougl - let's meditate 🙏 on the redesign of #40 this week and convene sometime next week with a more thorough discussion!

[mkcor]

Thanks, @chriddyp !

Strictly speaking / technically, gg2list is not private, because you can call it readily once you've loaded the "plotly" package, and we've not using @keywords internal in its docstring or anything.
But I would say that, culturally, it is, indeed.

Okay, I'll sync up with master and update files NEWS and DESCRIPTION.

[cpsievert]

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/61206866

On TravisCI, commit aff2997 was successfully merged with d9ecaf4 (master) to create 512b5b5. A visual testing table comparing d9ecaf4 with 512b5b5 can be found here:
http://ropensci.github.io/plotly-test-table/tables/512b5b501820c1e22133657ac4472e5b98a72bcc/index.html

[cpsievert]

I agree that these "backwards-incompatible" changes aren't a huge deal; but nevertheless, I like to follow the semantic versioning protocol, which would suggest to bump the version to 0.6.

Sorry I didn't mention that earlier. Again, not a big deal, but something to consider for the future.

[mkcor]

@cpsievert Great call! You are right. I care about versioning properly. Submitting #215