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

Handle 'since' in addition to 'page' for pagination response #2135

Merged
merged 3 commits into from Nov 27, 2021
Merged

Handle 'since' in addition to 'page' for pagination response #2135

merged 3 commits into from Nov 27, 2021

Conversation

brianlangdon
Copy link
Contributor

@brianlangdon brianlangdon commented Oct 21, 2021

pagination may return either 'page' or 'since' in the header. 'since' was not being handled

Fixes: #2112.

@google-cla
Copy link

google-cla bot commented Oct 21, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 21, 2021
@brianlangdon
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Oct 21, 2021
@codecov
Copy link

codecov bot commented Oct 23, 2021

Codecov Report

Merging #2135 (8825238) into master (b26fa8f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2135   +/-   ##
=======================================
  Coverage   97.79%   97.79%           
=======================================
  Files         112      112           
  Lines       10036    10041    +5     
=======================================
+ Hits         9815     9820    +5     
  Misses        154      154           
  Partials       67       67           
Impacted Files Coverage Δ
github/github.go 97.62% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b26fa8f...8825238. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @brianlangdon !
LGTM.

Awaiting second LGTM before merging.

@gmlewis gmlewis changed the title issue 2112 - handle 'since' when returned in the header when paging reponce Handle 'since' in addition to 'page' for pagination response Oct 25, 2021
@gmlewis gmlewis requested a review from wesleimp October 25, 2021 02:54
@brianlangdon
Copy link
Contributor Author

I've cleared the conflict introduced by PR #2154 would it be possible for somebody to approve and merge? @gmlewis , @wesleimp ?

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 23, 2021

I've cleared the conflict introduced by PR #2154 would it be possible for somebody to approve and merge? @gmlewis , @wesleimp ?

Thank you, @brianlangdon - can you please run go fmt on your file and push the changes?

@brianlangdon
Copy link
Contributor Author

@gmlewis, changes pushed I hope!, Thanks

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 24, 2021

Thank you, @brianlangdon ! LGTM.

I've cleared the conflict introduced by PR #2154 would it be possible for somebody to approve and merge? @gmlewis , @wesleimp ?

Please note that ALL other contributors to this repo are welcome to provide the second PR review/comment/approval that we need for merging.

Perhaps @Parker77 has time today to review.

Copy link

@Parker77 Parker77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 27, 2021

LGTM.

Thank you, @Parker77 !
Merging.

@gmlewis gmlewis merged commit c6b75c1 into google:master Nov 27, 2021
@lonre
Copy link

lonre commented Dec 5, 2021

Hi, @gmlewis @brianlangdon

It sees that this break https://docs.github.com/en/rest/reference/repos#list-commits with since params

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 5, 2021

Hi, @gmlewis @brianlangdon

It sees that this break https://docs.github.com/en/rest/reference/repos#list-commits with since params

Hi @lonre - could you please be more specific?

How did it break?

Can you please give an example of the response (before and after, or "working" and "broken")?

@lonre
Copy link

lonre commented Dec 5, 2021

Hi @gmlewis

with https://api.github.com/repos/Homebrew/homebrew-cask/commits?since=2021-12-04T10%3A43%3A42Z request,

the response NextPage and LastPage is 0, the pagination is broken

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 5, 2021

Thank you for more details, @lonre !
I'll re-open #2112 and show my findings there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

func populatePageValues can't handle API's returning 'since'
4 participants