Skip to content
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

Improve API client UX #35

Closed
3 tasks done
thekaveman opened this issue Oct 31, 2018 · 8 comments
Closed
3 tasks done

Improve API client UX #35

thekaveman opened this issue Oct 31, 2018 · 8 comments
Labels
enhancement New feature or request
Milestone

Comments

@thekaveman
Copy link
Contributor

thekaveman commented Oct 31, 2018

Right now the usage is a multi-step process including at least:

  1. get the providers registry
  2. filter registry for desired providers
  3. configure resulting providers with auth_token etc., assuming you know how to generate all the different types of tokens
  4. finally, use a ProviderClient to get_status_changes() or get_trips()

It is also somewhat confusing to have a ProviderClient that "manages" or queries multiple Providers at once. The code is probably over-complicated because of this as well...

This could all be simplified with some specific improvements to this class:

  • allow the providers keyword arg in both __init__ and the get_* methods to accept a string or array of string provider names. Internally handle filtering the registry (ProviderClient already can query and obtain the registry itself).

  • allow for a config parameter in __init__ and the get_* methods to pass in provider configuration, vs. having to configure them ahead of time.

  • consider removing support for "managing" multiple providers at once - one Provider, one ProviderClient.

@johnclary
Copy link

another use case: i would like to use the API client modules but am not interested in the db loading. from that perspective it may be logical to split the api/db purposes into separate packages.

i'll be spending more time with this project in the coming weeks. hope i can contribute.

@johnclary
Copy link

consider removing support for "managing" multiple providers at once - one Provider, one ProviderClient

+1

@thekaveman
Copy link
Contributor Author

another use case: i would like to use the API client modules but am not interested in the db loading. from that perspective it may be logical to split the api/db purposes into separate packages

I'm not sure I see the benefit here. The only dependency related to mds.db that isn't also used elsewhere is sqlalchemy; there isn't much to be gained in that regard from separating api and db.

Of course you don't have to import mds.db! Is there another concern that I'm not thinking of?

@johnclary
Copy link

johnclary commented Nov 21, 2018

True. I'm mostly thinking of the pip install experience. My install happens to be choking on geopandas, but I haven't spent any time on it yet (and I'd be surprised if that's hard to resolve). Definitely less of a concern if the dependencies are shared.

@johnclary
Copy link

johnclary commented Nov 21, 2018

as an aside, looks like geopandas dependency can easily be dropped. will open new issue/pr.

@thekaveman
Copy link
Contributor Author

as an aside, looks like geopandas dependency can easily be dropped. will open new issue/pr.

It's only used here:

collection = geopandas.GeoSeries([shape]).__geo_interface__

To convert a shapely.geometry.Shape into a GeoJSON object. Probably extreme overkill to use an entire library just for that 😆 Happy to accept a PR to remove that dependency.

This was referenced Nov 22, 2018
ezheidtmann added a commit to RideReport/mds-provider that referenced this issue Dec 3, 2018
ezheidtmann added a commit to RideReport/mds-provider that referenced this issue Dec 12, 2018
@ezheidtmann
Copy link

The work I'm doing in #46 and #45 is moving towards "one provider -> one ProviderClient". This seems to be independent of the two other items listed in this issue (provider names & provider config).

@thekaveman thekaveman added this to the MDS 0.3.0 milestone Apr 24, 2019
thekaveman added a commit that referenced this issue Apr 24, 2019
@thekaveman thekaveman added the enhancement New feature or request label Apr 24, 2019
@thekaveman
Copy link
Contributor Author

Thanks to #40 and #71 the above changes will be made, including the move to a single provider per ProviderClient.

Initialize with a provider name or identifier directly:

client_A = ProviderClient("ProviderA")
client_B = ProviderClient("da80482e-1118-48e8-bccc-38492960bd17")

# optionally pass in configuration data
client_C = ProviderClient("ProviderC", config={ "token": "private" })

Use a provider name or identifier at request time:

client = ProviderClient()

trips_A = client.get_trips("ProviderA")
trips_B = client.get_trips("da80482e-1118-48e8-bccc-38492960bd17")

# optionally pass in configuration data
trips_C = client.get_trips("ProviderC", config={ "token": "private" })

thekaveman added a commit that referenced this issue Apr 24, 2019
thekaveman added a commit that referenced this issue May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants