Skip to content

No auto load graph reference #643

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 14 commits into from
Closed

Conversation

theengineear
Copy link
Contributor

@theengineear theengineear commented Dec 30, 2016

Sorta a WIP since I've done this on top of the unify-api branch.

This:

  1. Uses @etpinard's commit to remove the graph_reference loading
  2. Adds some meta programming so we don't need to construct these classes on import.**

**See the commit message for more details.

Fixes #389, #613, #611, #527, #419

alexandresobolevski and others added 12 commits December 21, 2016 18:03
In general, we have cyclic import issues all over the place, this is one
easy fix and will help out in later commits.

Note that this maintains backwards compat due to how the the functions
are imported into `plotly.py`.
This is also some setup for a larger refact. Since v1 and v2 requests
handle errors differently, it’s easier if we simplify the api into our
errors so that the requests can go from `response` —> `python error` as
they see fit.
This was driving me nuts. We basically manually handle creating and
validating *each* api response inside each calling function. Even worse,
we *sometimes* raise a `PlotlyRequestError` and *sometimes* just bubble
up the `requests.exceptions.HTTPError` ;__;.

This does the following:

* Define an `api.v1` module that only includes `clientresp` (the only old
api method we still *need* to cling to)
* Define an `api.v2` module that includes all the new functionality of
our v2 api.
* Both `v1` and `v2` raise the same `PlotlyRequestError`, so that users
only need to catch *one* exception class in scripts.
Note that the `apigetfile` did some weird things to convert old-style
plotlyjs figures (e.g. `type: ‘histogramx’`) to new-style versions.

I wanted to ween us off the old api, so this makes the change from
`/apigetfile` —> `/v2/plots/[fid]/content?inline_data=true`.

The `_swap*` functionality was copied from `plotly/streambed` code,
directly from the backend’s implementation of `apigetfile`.
There is a circular import issue that needed to be fixed here as well.
This is allowed when setting the credentials *file*, however, trying to
set this inside a session used to fail.
The `requests` package kindly manages 2/3 compat for json for us, might
as well be consistent and use the same tool!

As a side note, I’d like to move away from `six` and depend directly on
`requests.compat`. I believe it has everything we need and then we can
ditch the `six` dep and know that we’re always in sync with whatever
requests is doing (which is really what we care about).
@theengineear
Copy link
Contributor Author

This is something for #642

Previously, we generated these *on import*, which is slow and it’s a
little sneaky. Users end up using classes that are not actually written
*anywhere* in our package.

Now that we only update the plot schema in version releases (no longer
on import), we can also just programmatically add in all the graph obj
classes. Instead of adding them to memory, we literally just print them
to the file.

This is some serious meta-programming… but, the resulting file will have
to be syntactically valid and pass our test suite, so I’m not *too*
worried about it.
@theengineear theengineear force-pushed the no-auto-load-graph-reference branch from 639832c to 6511aad Compare December 30, 2016 20:48
@theengineear
Copy link
Contributor Author

This doesn't break any backwards compat, right? I think I'm going to try to open against the current master since it will be able to get in much faster.

@theengineear
Copy link
Contributor Author

Closing in favor of #648

@theengineear theengineear deleted the no-auto-load-graph-reference branch January 5, 2017 22:24
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.

2 participants