search

CityofSantaMonica / mds-provider

Python tools for working with MDS Provider data
https://github.com/openmobilityfoundation/mobility-data-specification
MIT License
18 stars 20 forks source link

Provider client iterator #46

Closed ezheidtmann closed 5 years ago

ezheidtmann commented 5 years ago

Rebased on #45

Refactor provider client to allow consumers to iterate through pages, rather than waiting for all pages before continuing.

Seeks to maintain ProviderClient interface without substantive change.

thekaveman commented 5 years ago

I really like this idea, haven't dug into the code too much yet so I don't fully understand what's going on...

I just wanted to point you to #35, where we're discussing actually removing the whole "pass multiple providers into a ProviderClient" concept, in which case SingleProviderClient is a bit redundant. Do you have any thoughts on that approach?

ezheidtmann commented 5 years ago

Cool, just skimmed that issue and I believe I agree. Based on the name I would expect ProviderClient to be a client to a single provider.

ezheidtmann commented 5 years ago

I renamed the client classes in this PR

thekaveman commented 5 years ago

@ezheidtmann just leaving these comments here as well...

I'd like to take the approach suggested in #35, to clean up the existing class rather than renaming and introducing new classes. Did you want to refactor this PR towards this approach?

ezheidtmann commented 5 years ago

I believe that my latest push does that in ProviderClient, and also provides a migration path for consumers in MultipleProviderClient. That said, I'm happy to revise again if you can provide more details on how the latest push doesn't meet your goals.

thekaveman commented 5 years ago

Thank you. I think what I'm getting at is, I'd rather not introduce a new class and support a migration path so early in the game. Refactoring the existing ProviderClient, event breaking to some extent (removing support for multiple providers) is totally OK.

Can you also please rebase on dev? Some conflicting changes went in and will be released sooner than later.

ezheidtmann commented 5 years ago

Got it, thanks for that direction. I'll work on a breaking change with an iterator-based API over the next few days; I pushed some early thinking in #45

thekaveman commented 5 years ago

Hey @ezheidtmann. The latest release has evolved the library to the point that I think this would 1) be much easier to implement, 2) need to be re-done anyway.

Closing this PR, but would welcome a new one. Thank you!