-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
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.
639832c
to
6511aad
Compare
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. |
Closing in favor of #648 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorta a WIP since I've done this on top of the
unify-api
branch.This:
**See the commit message for more details.
Fixes #389, #613, #611, #527, #419