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

Add ListOptions to GetCommit and CompareCommits to support pagination #1960

Merged
merged 8 commits into from
Jul 13, 2021

Conversation

Saaarah
Copy link
Contributor

@Saaarah Saaarah commented Jul 9, 2021

No description provided.

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jul 9, 2021
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.

Thanks, @Saaarah.

Please make sure to run unit tests before submitting PRs.
You will need to modify the unit tests since the API has changed with this PR.

Please check out surrounding tests to see how pagination is also tested. Thanks.

github/repos_commits.go Show resolved Hide resolved
@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Jul 10, 2021
@Saaarah
Copy link
Contributor Author

Saaarah commented Jul 12, 2021

Thanks, @Saaarah.

Please make sure to run unit tests before submitting PRs.
You will need to modify the unit tests since the API has changed with this PR.

Please check out surrounding tests to see how pagination is also tested. Thanks.

Hi Glenn, thanks for reviewing my pull request and I've added the new parameters in the two API's unit tests. Thanks again!

@gmlewis
Copy link
Collaborator

gmlewis commented Jul 12, 2021

Hi @Saaarah - it looks like there are some calls to these APIs in the examples, if you could please take a look and fix them.

Here is the error:

Run go vet ./...
# github.com/google/go-github/v37/example/commitpr
vet: example/commitpr/main.go:128:97: too few arguments in call to client.Repositories.GetCommit
Error: Process completed with exit code 2.

@Saaarah
Copy link
Contributor Author

Saaarah commented Jul 12, 2021

Hi @Saaarah - it looks like there are some calls to these APIs in the examples, if you could please take a look and fix them.

Here is the error:

Run go vet ./...
# github.com/google/go-github/v37/example/commitpr
vet: example/commitpr/main.go:128:97: too few arguments in call to client.Repositories.GetCommit
Error: Process completed with exit code 2.

Hi Glenn, I've fixed the example. Sorry for not updating all affected code, it'll be nice to have a small section in readme.md to ask contributors to run all necessary tests prior to creating pull request, which will be really helpful for first timer contributors.

@gmlewis
Copy link
Collaborator

gmlewis commented Jul 12, 2021

it'll be nice to have a small section in readme.md to ask contributors to run all necessary tests prior to creating pull request, which will be really helpful for first timer contributors.

This is already covered in our CONTRIBUTING.md doc:

  5. Please run:
     * `go generate github.com/google/go-github/...`
     * `go test github.com/google/go-github/...`
     * `go vet github.com/google/go-github/...`

@gmlewis
Copy link
Collaborator

gmlewis commented Jul 12, 2021

It now looks like gofmt needs to be run on the example code:

Run diff -u <(echo -n) <(gofmt -d -s .)
--- /dev/fd/63	2021-07-12 15:58:15.735418376 +0000
+++ /dev/fd/62	2021-07-12 15:58:15.735418376 +0000
@@ -0,0 +1,12 @@
+diff -u example/commitpr/main.go.orig example/commitpr/main.go
+--- example/commitpr/main.go.orig	2021-07-12 15:58:16.035434090 +0000
++++ example/commitpr/main.go	2021-07-12 15:58:16.035434090 +0000
+@@ -127,7 +127,7 @@
+ 	// Get the parent commit to attach the commit to.
+ 	opt := &github.ListOptions{
+ 		PerPage: 10,
+-		Page: 1,
++		Page:    1,
+ 	}
+ 	parent, _, err := client.Repositories.GetCommit(ctx, *sourceOwner, *sourceRepo, *ref.Object.SHA, opt)
+ 	if err != nil {
Error: Process completed with exit code 1.

@Saaarah
Copy link
Contributor Author

Saaarah commented Jul 12, 2021

it'll be nice to have a small section in readme.md to ask contributors to run all necessary tests prior to creating pull request, which will be really helpful for first timer contributors.

This is already covered in our CONTRIBUTING.md doc:

  5. Please run:
     * `go generate github.com/google/go-github/...`
     * `go test github.com/google/go-github/...`
     * `go vet github.com/google/go-github/...`

Ah thanks for pointing out! I've formatted the file and run all the commands listed above. Thanks Glenn!

@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #1960 (3d883f7) into master (cda3c0f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1960   +/-   ##
=======================================
  Coverage   97.84%   97.84%           
=======================================
  Files         105      105           
  Lines        6809     6815    +6     
=======================================
+ Hits         6662     6668    +6     
  Misses         80       80           
  Partials       67       67           
Impacted Files Coverage Δ
github/repos_commits.go 100.00% <100.00%> (ø)

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 cda3c0f...3d883f7. Read the comment docs.

@gmlewis gmlewis requested a review from wesleimp July 12, 2021 16:56
example/commitpr/main.go Outdated Show resolved Hide resolved
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, @Saaarah !
LGTM.

Awaiting second LGTM before merging.

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 gmlewis merged commit 0184fb1 into google:master Jul 13, 2021
xun-guo-anzx added a commit to xun-guo-anzx/terraform-provider-github that referenced this pull request Aug 18, 2021
go-github released the Go client library to [38.1.0][1] recently.

API changes includes:
* Add ListOptions required by GetCommit method. As described in change[2]
* Add followRedirects required by GetBranch method and set it to false as start point. As described in change[3]

[1]: https://github.com/google/go-github/releases/tag/v38.1.0
[2]: google/go-github#1960
[3]: google/go-github#1901
xun-guo-anzx added a commit to xun-guo-anzx/terraform-provider-github that referenced this pull request Sep 6, 2021
go-github released the Go client library to [38.1.0][1] recently.

API changes includes:
* Add ListOptions required by GetCommit method. As described in change[2]
* Add followRedirects required by GetBranch method and set it to false as start point. As described in change[3]

[1]: https://github.com/google/go-github/releases/tag/v38.1.0
[2]: google/go-github#1960
[3]: google/go-github#1901
xun-guo-anzx added a commit to xun-guo-anzx/terraform-provider-github that referenced this pull request Sep 6, 2021
go-github released the Go client library to [38.1.0][1] recently.

API changes includes:
* Add ListOptions required by GetCommit method. As described in change[2]
* Add followRedirects required by GetBranch method and set it to false as start point. As described in change[3]

[1]: https://github.com/google/go-github/releases/tag/v38.1.0
[2]: google/go-github#1960
[3]: google/go-github#1901
suzuki-shunsuke added a commit to suzuki-shunsuke/tfcmt that referenced this pull request Sep 24, 2021
* google/go-github#1960
* https://docs.github.com/en/rest/reference/repos#get-a-commit

> Note: If there are more than 300 files in the commit diff, the response will include pagination link headers for the remaining files, up to a limit of 3000 files.
> Each page contains the static commit information, and the only changes are to the file listing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). 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.

None yet

4 participants