Requests reloaded

[mineo]

This is #123 again, rebased on top of current master and with passing tests ✨

Note that I've bumped the requests dependency to 2.5.0 because that's the first version that allows using urllib3's Retry objects for max_retries on the HTTPAdapter and (afaik) that's the only way to implement retries on a specific set of HTTP status codes. t

Especially the retry/exception part could use a second pair of eyes to make sure everything works as expected and if you have some scripts that make heavy use of pymbngs, please run them with this pull request and see if nothing breaks.

[sampsyo]

I could have this wrong, but it looks on my machine like RetryError is in the requests.exceptions module, not just in requests:

>>> requests.__version__
'2.9.1'
>>> requests.RetryError
Traceback (most recent call last):
  File "<input>", line 1, in <module>
    requests.RetryError
AttributeError: 'module' object has no attribute 'RetryError'
>>> requests.exceptions.RetryError
<class 'requests.exceptions.RetryError'>

Also, I noticed that RetryError inherits from RequestException, so it would have the same effect to just fall through to the next except handler.

[sampsyo]

Woohoo! Thanks for reviving this. I reviewed the diff and it looks great to me—I can give it a try with a beets import sometime soon too.

[alastair]

Cool! I've mellowed in my beliefs since the last time this patch was proposed. I see no problem in going for it. @mineo, do you want to coordinate merging?

[mineo]

@alastair Sure, I can do that. Do you want me to merge it to master and make the next version the API change one or keep this on a separate branch on the off-chance that we'll get around to tackling some of the other API change tickets at some point in the near future?

[alastair]

Hmm. If we do this right, this change shouldn't affect the external API, right?

I'd really like to get a release out adding features for all of the outstanding schema changes before we start playing with the external API.

What other options are there? We could run 2 APIs in parallel, and you can "turn on" the old one by changing an import? It was 3 (!) years ago that I last said it (#116 (comment)), but I still don't like APIs changing under me from one version to another, even if the version number is "not stable". What do you think?

[mineo]

The only externally visible change is the one mentioned in CHANGES, the cause of WebServiceErrors and I don't think we can easily translate those back to the old exceptions.

To be honest, I'd rather keep things as they are and shelve this pull request until we're changing interfaces for some other reason instead of maintaining 2 APIs.

[sampsyo]

Hi! I'd like to revive the requests discussion if folks here are interested. In particular, we'd really like to switch beets to use OAuth2 instead of storing a password in plaintext. This has turned out to be relatively easy to prototype using requests (see the beets PR above) and relatively hard to do without it.

So, instead of dropping python-musicbrainzngs for the part of the code that needs to use OAuth2, we'd love to merge that support into the library. How are folks feeling about a requests-based release that, as its raison detre, also adds OAuth authentication? We (speaking collectively for the beets project) would be happy to contribute as much code as we can if people are amenable to seeing the such a release in a "soonish" time frame.

[Freso]

requests also has SNI support, right? So the library could default to being all HTTPS and not worry about Python <2.7.9? Either way, my vote (FWIW) is to just do it. As mentioned in #123 the library is still a version 0.x, so not officially stable (going by SemVer), and the backwards incompatibility incurred with this switch is minor.

[sampsyo]

The requests FAQ seems to indicate that it probably supports SNI, depending on the dependencies:
http://docs.python-requests.org/en/master/community/faq/

Anyway, yes! That would be a great knock-on effect. What do you think, @alastair?

[alastair]

Hey everyone,
Thanks for picking this up again, @sampsyo. Let's do it!
Since there are ways of making SNI work if the correct packages are installed, I think we could make this work on older pythons with minimal effort (<2.7.9 would be the important ones)

Do we know what the API differences are going to be, if any? I thought that maybe #85 would be a problem, but this happens after the data is downloaded, and so I think that switching to requests won't affect this.

I would argue that the library has been around too long to use semver as an excuse for making incompatible changes without a good reason. If we can show that changing to requests results in no difference in the way that it's called, then we should merge this as soon as possible (e.g. perhaps if tests still pass and beets can show that there was no required changes to the API to work with requests)

[sampsyo]

Cool! I'll give this a try, do a thorough code review, and run some tests. I think the only visible change is the one @mineo mentioned above: the cause field of exceptions, which expose the underlying network exceptions. Seems pretty minor.

I'll start adding OAuth support once that's ready in a separate PR.

[mineo]

Ignoring the typos in the docstring above, requests recently unvendored all its external dependencies, so this code here should be double-checked (see http://docs.python-requests.org/en/latest/community/updates/#id9).