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.

Sign up for GitHub

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

Jump to bottom

Provider client iterator #46

Closed
wants to merge 4 commits into from
Closed

Provider client iterator #46

wants to merge 4 commits into from

Conversation

ezheidtmann
Copy link

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.

@ezheidtmann
Fake server & basic tests against it
37a1661
@ezheidtmann
Remove broken init code
e563413
@ezheidtmann
Expose single-provider page iterator
f2648c5 
A little refactoring to allow consumers to iterate pages from each
provider.  Intention is to maintain ProviderClient behavior except for
these changes:

 - don't request a second page if the first page is empty (previously
   would have made a second request if `next_url` was present on first
   page)
 - don't return any results for a provider if any pages failed
   (previously would have returned partial results if a page other than
   the first failed)
@thekaveman
Copy link
Contributor

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
Copy link
Author

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.

@thekaveman thekaveman mentioned this pull request Nov 30, 2018
Closed
@ezheidtmann
Copy link
Author

I renamed the client classes in this PR

@ezheidtmann ezheidtmann mentioned this pull request Dec 3, 2018
Closed
@thekaveman
Copy link
Contributor

@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
Copy link
Author

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.

ezheidtmann added a commit to RideReport/mds-provider that referenced this pull request Dec 12, 2018
@ezheidtmann
Rename ProviderClient -> MultipleProviderClient
6bd9070 
Perhaps an improvement to names, re:

CityofSantaMonica#46
CityofSantaMonica#35
This was referenced Dec 12, 2018
Closed
Closed
@thekaveman
Copy link
Contributor

thekaveman commented Dec 14, 2018 •
edited
Loading

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
Copy link
Author

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 thekaveman changed the base branch from dev to master December 31, 2018 19:23
@thekaveman
Copy link
Contributor

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!

@thekaveman thekaveman closed this Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ezheidtmann @thekaveman