Conversation
|
Can I try this? I have written basic |
Tokens api support
|
@sgillies This is ready for review. I added some docs/tests on top of the work done by @iAmMrinal0 in #198 - should be close to ready for 🚀 EDIT: apparently github won't let you to review your own PR 😛 - assigned it to you instead. |
sgillies
left a comment
sgillies
left a comment
There was a problem hiding this comment.
The code looks good. I like the method names and argument order. I've got one argument renaming suggestion and a concern that the token checking API is a bit obscure.
Oops, since this is put onto my own branch, I can't really review it @perrygeo.
docs/tokens.md
Outdated
| ``` | ||
|
|
||
|
|
||
| ## Create a permanet token |
| Your Mapbox access token should be set in your environment; | ||
| see the [access tokens](access_tokens.md) documentation for more information. | ||
|
|
||
| The Mapbox username associated with each account is determined by the access_token by default. All of the methods also take an optional `username` keyword argument to override this default. |
There was a problem hiding this comment.
@perrygeo I just noticed that we're using an account keyword arg in uploads. Should we follow suit here?
There was a problem hiding this comment.
Yeah, we should make that consistent. But looks like the main API docs use username throughout; maybe the solution is to make the change to uploads, s/account/username/g?
There was a problem hiding this comment.
Okay we'll follow up there.
| ```python | ||
|
|
||
| >>> service.check_validity().json()['code'] | ||
| 'TokenValid' |
There was a problem hiding this comment.
I'm confused about where the token gets passed to the method. I think it might be more clear like check_validity(token), no?
A quicker check for validity would be nice, too (like is_valid(token) --> True/False, but that's not really the style of this API.
| ``` | ||
|
|
||
| Note that this applies only to the access token which is making the request. | ||
| If you want to check the validity of other tokens, you must make a separate instance of the `Tokens` service class using the desired `access_token`. |
There was a problem hiding this comment.
Answers my question above. I still don't think the current implementation feels quite right.
There was a problem hiding this comment.
Agreed that the check_validity method feel a bit off. Similarly the list_scopes method.
They fit the http-response-first style of the rest of this module but usability suffers a bit. Still 🤔 about potential solutions...
There was a problem hiding this comment.
re-reading the docs for this endpoint (https://www.mapbox.com/api-documentation/#retrieve-a-token) it occurs to me that
- validity is not a binary choice, there are five possible token codes.
- the endpoint is intended to retrieve all metadata about the token, including the token code but also the authorization id, etc.
check_validitymight be a bit of a misnomer.
As such, I'm ok with leaving the current implementation as is for the purposes of this PR.
But perhaps we need to rename it. Consistency with the HTTP api docs would suggest retrieve_token?
There was a problem hiding this comment.
And regarding the funky implementation detail of needing to create an instance of Tokens before checking validity, maybe we could write some @classmethods which do the instantiation silently, presenting a simpler interface that accepts a token as an argument?
Tokens.retrieve(token)
There was a problem hiding this comment.
@perrygeo yes, that's the way to go. I think get or get_token is better than retrieve, more true to the HTTP nature of the API.
|
Okay, this PR is mostly still on track. TODO:
|
3 participants
Very preliminary. Towards resolving #156.