Skip to content

2.0.0 #658

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

Merged
merged 26 commits into from
Jan 19, 2017
Merged

2.0.0 #658

merged 26 commits into from
Jan 19, 2017

Conversation

theengineear
Copy link
Contributor

@theengineear theengineear commented Jan 11, 2017

Aight, we're set to merge this. I'm going to ask for a final round of review and then merge and setup a pre-release for this before I push anything new up to PyPI.

Closes #642

@theengineear
Copy link
Contributor Author

@chriddyp @Kully @cldougl is there any reason for us to support 1.X anymore after we do a major version bump? Bigger projects often support (and continue to update minor versions on) all major versions. (see the stable/X branches in django for an example). I don't see a need for us to do this and we should always be able to go back in time, check out a previous version and cherry-pick in a change if necessary.

In other words, I'd like to keep our versioning linear if possible, each new version should be a higher number than the last. I suppose we can always change this later by pulling old commits into new branches, etc, etc.

Again, I'm suggesting that we merge 2.0.0 directly into master and not manage 1.X and 2.X in separate branches or something.

@Kully
Copy link
Contributor

Kully commented Jan 11, 2017

I feel like it would be pretty annoying to maintain both branches. I recall a lot of organization stuff (FigureFactory) which won't affect the users' experience and more additions which make 2.0.0 sounds better.

@theengineear
Copy link
Contributor Author

I feel like it would be pretty annoying to maintain both branches.

Yup, it would be. Not supporting multiple, concurrent versions is definitely my vote, but I just want to get consensus on our approach.

@cldougl
Copy link
Member

cldougl commented Jan 12, 2017

+1 for not continuing to support version 1.X
*We'll have to update the docs as well.

@theengineear
Copy link
Contributor Author

Great! Consensus-enough! Then I'm going to merge 2.0.0 to master today and get it up on PyPI.

@theengineear
Copy link
Contributor Author

Just a couple things that I'm up in the air on:

  1. I'm considering failing in FF methods if you don't have numpy installed. I'd want to do that in 2.0.0 as a single backwards incompatible change.
  2. I'd still like to do sign_in validation. It seems like a helpful thing and is definitely not backwards compat. I may back-burner the merge until the weekend when I can grab some free cycles to finish this up.

Seem reasonable?

@theengineear
Copy link
Contributor Author

Okie dokie. Heres' the state of 2.0.0:

I'll version-bump when all that's through ;)

alexandresobolevski and others added 23 commits January 18, 2017 13:13
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).
The old location is now deprecated.
Ok the changes in default args are more than style changes… Just a heads
up.
This is a stand-alone module to do the following:

* Don’t import optional things until we need them
* Keep all the optional imports centralized
I can’t believe that this wasn’t tested anywhere before!
Note that we don’t necessarily *require* `numpy` for in all the FF
methods. We’re just looking to make the compatibility-breaking
change so that we can depend on it in `2.0.0`.

Also note that I’d forgotten to include `figure_factory` in setup.py
before ;__;.
@theengineear
Copy link
Contributor Author

Aight, final tests are happening now!

@chriddyp @cldougl @Kully , any final reservations? I'll create a pre-release tag for this and may ask for a bit more manual testing after this hits master.

@cldougl
Copy link
Member

cldougl commented Jan 18, 2017

no reservations!
and I'm happy to help manual test too

@theengineear
Copy link
Contributor Author

OK, done waiting, I think the next step needs to be manual testing. Merge time!

@theengineear theengineear merged commit 0f5f7cc into master Jan 19, 2017
@theengineear theengineear deleted the 2.0.0 branch January 19, 2017 09:01
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