Redesign the API of Client

[Ceasar]

Hey @kylehg, finally got around to looking at this today. Made some changes, detailed below.

This commit redesigns the API of the Client class so that:

- Client._request() is an independent function, _request(). This reduces
  complexity by reducing the information that _request() has access to
  (i.e. access_token, instead of all instance variables).

- Client.exchange_authorization code() no longer has a return value. It
  also sets a new instance variable, Client.refresh_token, which holds
  the refresh token.

- Client.exchange_refresh_token() no longer takes a refresh token as an
  argument, since Client.refresh_token provides that information.

This commit also adds some documentation to the Client class and
reformats existing documentation to make it more readable.

Also, some more general things to consider:

• You don't have any tests, which makes it harder for people to contribute and to maintain. (I can't with confidence say this change won't break any existing behavior.)
• The Medium REST API does not appear to be REST-ful, or at least, we're not accessing it in a RESTful way here. One of the constraints REST imposes, unofficially known as "Hypermedia As The Engine Of Application State", says that you should be able to access any resource from the entry point (in this case https://api.medium.com/v1). I didn't check the API to verify if you follow that constraint, but if you do, clients should access resources by finding them from the entry point rather than constructing URLs as we do here. This allows the web service maintainer to manipulate URLs in the future, which make the service easier to maintain if it turns out you need to change it.
• In general, you should try to follow the command-query separation principle since it makes class interfaces simpler to understand if slightly more verbose.

Otherwise though, hardly noticed you hadn't used Python in a while. 👍

[kylehg]

This is awesome, @Ceasar! You're definitely right about the tests—I'm gonna try to add some today or tomorrow so that we can check these changes against them.

I'll do a closer review this afternoon, but at first blush I like your changes to the interface and exchange_token methods.

Your point about being able to access any resources from the root is a pretty interesting one, I hadn't heard that idea before, but we don't do that now and it's probably out of scope for the time being.

[Ceasar]

If you ever do get around to it (and it's not that hard), the Github API is a great example of the HATEOAS principle. Each response includes links to other resources and the index contains links to each kind of resource.

[majelbstoat]

Hey @Ceasar, appreciate you taking the time to help us improve our SDK!

You're right that, per Fielding's dissertation and later writing, the lack of HATEOAS technically means the API isn't RESTful. However, I use it in the documentation in a colloquial sense to define an API that is centered around resources and collections, where actions are expressed through HTTP verbs, differentiated in that way from a /createPost-like RPC API. Now, the SDK itself might itself be RPCish, but the endpoints to which it is calling are all CQS, and built around resources and collections. PUTs, GETs and DELETEs are idempotent, POSTs are not, etc. (I've come to believe that an RPCish client built on RESTish endpoints is actually the most useful and flexible architecture. Here, I guess, RPCish client basically just means 'has methods'!)

HATEOAS is fine in theory, though in my experience doesn't actually grant the flexibility that an API designer might wish. Frankly, third-party clients are likely to hardcode route patterns in their code, regardless of whether they are specified, for a whole bunch of reasons, and while the server-side developer might well have the moral high ground when making a change to those routes, it's still going to annoy client developers when their code breaks. Ultimately, it doesn't matter in the end if the client is even set up to expect route changes; typically those kinds of changes are also accompanied by parameter or payload changes, which the client won't know how to send properly. Indeed, the idea that a server and client can be totally decoupled has never proven itself out in any way that I've seen in the real world. (Keen to hear counter examples if you have them.)

I take the view that a slow and deliberate evolution of the API using versioning is a more pragmatic approach, which encourages thoughtful up-front design befitting of something which will have to be stable for a long time. We plan to handle non-backwards-compatible API changes through iterations of the /v1/ prefix.

And as a tool for documentation, HATEOAS is... fine, but actual good documentation is better (we can certainly do better here, and will). Github's root URL is useful to a point, but when I'm programming against it, I'm not going to that URL, I'm going to the documentation every time. (What are the valid values for 'sort'? is a user the username, or the userId?)

[Ceasar]

Hey @majelbstoat, thanks for the thoughtful comment.

I agree with you on pretty much everything: you're going to need docs to explain the API anyway, and like a database schema, clients are coupled to the semantics of your resource representations and we need to give them just as much as, if not more, thought.

However there is one very good reason to use hypermedia in my opinion-- and that's because it's much easier to write and use SDKs if the API follows HATEOAS. (I disagree that RPC clients are better.) The first client I discovered which convinced me of this was created by Leonard Richardson while at Launchpad. (There's a great little explanation behind its origin here.) It works like this:

>>> from launchpadlib.launchpad import Launchpad
>>> launchpad = Launchpad.login_anonymously('testing', 'production')
>>> launchpad.bugs[1].title
u'Microsoft has a majority market share'
>>> launchpad.bugs[1].self_link
'https://api.staging.launchpad.net/beta/bugs/1'
>>> launchpad.bugs[1].web_link
'https://bugs.staging.launchpad.net/bugs/1'
>>> pitti = launchpad.people['pitti']
>>> pitti.display_name = 'A user who edits through the Launchpad web service.'
>>> pitti.lp_save()

The interesting part is launchpad.bugs[1].title. While you can emulate this behavior without a hypermedia API, it's naturally much more code. I have a reference client that, with a little configuration, can read (but not write) any hypermedia API here, which is less than 100 lines of code, and which would work for the entire API.

It's also been my experience that it's easy to consume hypermedia APIs using a library like Requests without any client, which made me end up abandoning that client that I wrote since it hardly saved any typing.

[majelbstoat]

That's interesting. I can't promise URL links like that will make it into the API any time soon, but I'll certainly be taking a look at your reference client and seeing what makes sense.

[kylehg]

Can you leave the assignment as a separate line, like in exchange_authorization_code? This makes for a lot going on in one statement

[Ceasar]

Yeah, good call. I don't like the idea of assigning anything to a variable named data, but assigning to tokens would make this simpler.

[kylehg]

Hey @Ceasar, thanks again for this PR! I def like the changes to the exchange_ methods, though I'd rather keep _request as a method (see comment). Aside from that just a few nits.

Also I added some tests so you now have something to test against—I even added some commented tests for your changes—but you'll have to sync against master to pick those up.

[kylehg]

Also @Ceasar can you sign the CLA so we can accept your contribution?

[huckphin]

Hello @Ceasar: Thanks for your contribution. Can I ask you to sign our CLA so that we can accept your contribution? Also, it looks as though you will need to update your branch as there are currently merge conflicts.

Let me know when I should take another look at this. Thank you!