Skip to content

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
Contributor

@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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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