Skip to content

API redesign #217

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

Closed
wants to merge 54 commits into from
Closed

API redesign #217

wants to merge 54 commits into from

Conversation

cpsievert
Copy link
Collaborator

I couldn’t help myself and took a stab at the redesign discussed in #40.

Some of the big changes include:

  1. No more list$function() interface. The new interface should be more intuitive to R users and resolves documentation issues (see Define parameters for plotly() #206). See the example section in help(plotly) for an overview of the new interface.
  2. Credentials/configuration are managed via environment variables so file permissions shouldn’t be an issue anymore (see Unable to set credentials, 'No such file or directory' #204). Users can still pass their username and API key to plotly() (and they will be automatically stored as environment variables). Also, the signup() function will (by default) try to store these variables in ~/.Rprofile so they will be loaded upon startup.
  3. Uses httr instead of RCurl to communicate with the plotly API. As a result, the source code is much simpler and provides some useful utilities for turning HTTP errors into R errors.
  4. Leverages the knitr::knit_print() S3 generic to automatically embed plotly iframes in R Markdown files. See test-plotly-knitr.R for a simple example.
  5. IPython users will have to use embed_notebook() to embed plotly frames in IPython (I haven't tested this yet, it'd be great if others could test this functionality).

Since this is such a drastic change to API, merging this would require bumping to 1.0.0. I’d be happy to work on shiny bindings as well (I’ve done this for several other R packages), but that might be appropriate for another pull request.

@cpsievert
Copy link
Collaborator Author

Since there are backwards incompatible changes, some changes will have to be made to the build script in plotly-test-table. I'll be experimenting with this over on my fork and might take this as opportunity to fix #209 and simplify the build process.

@mkcor
Copy link
Contributor

mkcor commented May 8, 2015

I must say that development has been strongly guided and influenced by the Python library... For instance, tools.R was a direct translation of https://github.com/plotly/python-api/blob/master/plotly/tools.py! Likewise, resorting to the signup function has been deprecated.

@cpsievert
Copy link
Collaborator Author

I'd be disappointed if this PR falls on deaf ears because it doesn't mimic the Python implementation...it's considerably simpler for both users and developers.

Why was the signup() function deprecated?

Just FYI, it looks like the os module can handle environment variables as well -- http://nbviewer.ipython.org/github/lightning-viz/lightning-example-notebooks/blob/master/images/images.ipynb

@theengineear
Copy link

@cpsievert , the signup() function was deprecated because we were seeing code go public that reveals private information (api keys). The easiest way we came up with to fix this pattern was to try and nudge folks into using the credentials/config files.

As for environment variables. That seems like a totally reasonable approach. If we do it for one library, the others should all support it though as the idea is transparent cross-collaboration between languages.

@theengineear
Copy link

Could you split out the credentials stuff into a different PR so the rest doesn't get held up?

@cpsievert
Copy link
Collaborator Author

the signup() function was deprecated because we were seeing code go public that reveals private information (api keys).

At least for the R package, it'd be easy to not relay the API message to avoid that issue. I like the signup() function, because to get started, all we do is:

library(plotly)
signup("me", "[email protected]")
q <- qplot(1:10)
plotly(q)

Could you split out the credentials stuff into a different PR so the rest doesn't get held up?

That might be awkward, but I'll think about it. Considering the integration with plotly-test-table, it will go much more smoothly if we do everything at once. Give me a couple days and I'll have a new build process for the test table which will be self-contained (within this package), more efficient, and easier to maintain.

@cpsievert
Copy link
Collaborator Author

If we do it for one library, the others should all support it though as the idea is transparent cross-collaboration between languages.

I can certainly see the benefit of sharing credentials between languages. If you'd like to keep the current approach of writing credentials to disk like this, just let me know, and I can add some support for reading/writing to those files (and fall back on environment variables if we don't have file permissions).

@bpostlethwaite
Copy link
Member

We are in the process of rolling out v2 of our API https://api.plot.ly/v2/ (60% complete, 100% complete in early June) and I would suggest any rewrite utilize them as existing calls will begin to return deprecation warnings. Things like exposing kwargs as an API parameter are, thankfully, a thing of the past.

Let me know if there is anything we can do to help introduce these new APIs. We will begin to roll them across all of our Libraries in June.

@cpsievert
Copy link
Collaborator Author

That's great to know, and I plan on waiting for v2 before merging this, thanks @bpostlethwaite!

The biggest thing for me is having the equivalent of the "clientresp" resource in the old API. It looks as though that will go under here https://api.plot.ly/v2/plots, but it's not quite ready yet?

@bpostlethwaite
Copy link
Member

@cpsievert yes, that is correct. ClientResp's replacement plots is the most work and will likely not land until early June.

@theengineear and @BRONSOLO are rolling it out and can let you know, and take questions, once it is complete.

@cpsievert cpsievert mentioned this pull request Jun 3, 2015
@cpsievert cpsievert closed this Jun 3, 2015
@cpsievert cpsievert deleted the carson-redesign branch August 1, 2015 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants