search

abcxyz / team-link

Apache License 2.0
1 stars 0 forks source link

Make paginated GitHub API call generic #33

Closed lock14 closed 4 months ago

lock14 commented 4 months ago

fixes #6

sqin2019 commented 4 months ago

I'm not sure about the benefit of making this generic. The new code seems less straightforward than the existing code. What are we gaining?

I believe the purpose for this should be we might reuse the generic pagination call especially when we call github, it is TODO from Seth's comment https://github.com/abcxyz/team-link/pull/5#discussion_r1521658357 @sethvargo

yolocs commented 4 months ago

I believe the purpose for this should be we might reuse the generic pagination call especially when we call github, it is TODO from Seth's comment #5 (comment) @sethvargo

I mean if it's truly like what Seth suggested, there should be less code when the generic function is used. But here we're having more code...

sethvargo commented 4 months ago

I mean if it's truly like what Seth suggested, there should be less code when the generic function is used. But here we're having more code...

Correct, this is more what I was trying to say: https://github.com/abcxyz/team-link/pull/34

lock14 commented 4 months ago

Its not that much extra code at the call site, maybe 5 or so extra lines. Most of the extra lines are due to fact that we are defining an anonymous function in place and an extra error check from the Paginate function itself.

It felt silly to make a generic github paginate function when it wouldn't be that much more work to make a completely generic paginate function that would work for other use cases (another API, database paging, etc.).

Also I'm not sure I agree with Seth's approach of de-duplicating inside the Paginate function. We want to de-duplicate for these particular teamlink calls, but its possible that in other use cases the caller would want the Paginate function to simply return the data as given. De-duplication can always happen after the Paginate call.

sethvargo commented 4 months ago

I dropped the generics and the sorting in #34

lock14 commented 4 months ago

I'm closing this in favor of #34.