search

mapbox / mapbox-sdk-rb

A Ruby interface to Mapbox APIs.
Other
59 stars 32 forks source link

Add support for Tokens API endpoints #41

Open morgant opened 5 years ago

morgant commented 5 years ago

A client of mine has a Ruby app that needs to manage their MapBox API tokens, so it'd be helpful if the Ruby MapBox SDK supported the Tokens API.

morgant commented 5 years ago

I'm currently exploring adding such support.

morgant commented 5 years ago

Okay, I think I have my implementation feature-complete (with the exception of temporary tokens) and ready to submit a PR, but I've got a couple questions first:

1) I stuck with this gem's method naming conventions, so #token_get, #tokens_list, #token_create, #token_update, #token_delete, #scopes_list, but would it be preferable to use #get_token, #list_tokens, etc.?

2) Some methods use a username parameter, but would it be preferable to make it an accessor like access_token?

3) I added tests, but they actually create/update/delete tokens (the tests clean up after themselves). Should I remove/comment out the tests?

morgant commented 5 years ago

@samfader Any input is welcome.

samfader commented 5 years ago

@morgant Great news!

  1. I think the latter is better (#get_token). Most of the other endpoints in this gem don't have similar methods, which ones were you referring to when thinking of naming conventions?
  2. I would keep it as a parameter.
  3. Is it possible for you to mock those tests instead?
morgant commented 5 years ago

@samfader Thanks!

  1. I agree the latter makes more sense and would align better with the JS SDK method names, so I'll swap them. Geocoding was the only one that had multiple methods, so I just followed that (geocode_forward & geocode_reverse).

  2. Sounds good, I'll leave it as-is in that regard.

  3. I've not done much in the way of mocking outside of RSpec & FactorBot (née FactoryGirl), but it looks like you're including Mocha, so I'll take a look at it.

samfader commented 5 years ago

Ah, gotcha re: 1. Yeah, I think just stick with what makes the most sense given that it's not a widespread design right now.

Re: 3, I don't think it's necessarily the end of the world if your tests are actually deleting/creating tokens, but I think it's best practice to try to mock instead.

morgant commented 5 years ago

@samfader Methods are renamed. I agree, better to mock for tests.

morgant commented 5 years ago

I haven't been able to find time to try to convert the tests to use mock HTTP requests/responses, so I've created PR #43 as-is. Maybe we can open a separate issue for updating the tests, if that's agreeable and the rest of the PR looks appropriate.

wubini commented 5 years ago

hi @morgant, are you planning to update the tests to use mock HTTP requests/responses if #43 won't be approved otherwise? thanks for doing this work!

morgant commented 5 years ago

@warsucks Unfortunately, I'm not familiar with implementing mock HTTP requests/responses. I'm happy to take pointers and give it a shot, but my initial research seemed like a bigger solution was likely needed (something akin to https://github.com/rebelidealist/stripe-ruby-mock).