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

Add ListOptions for listing user migrations #2417

Merged
merged 2 commits into from Aug 22, 2022
Merged

Add ListOptions for listing user migrations #2417

merged 2 commits into from Aug 22, 2022

Conversation

amitsaha
Copy link
Contributor

Fix for #2416

@amitsaha amitsaha changed the title Fix #2416 Pagination for list user migrations Fix #2416 Add ListOptions for list user migrations Jul 26, 2022
@amitsaha amitsaha changed the title Fix #2416 Add ListOptions for list user migrations Fix #2416 Add ListOptions for listing user migrations Jul 26, 2022
@codecov
Copy link

codecov bot commented Jul 26, 2022 •
edited

Codecov Report

Merging #2417 (a518e88) into master (01c8feb) will decrease coverage by 0.02%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #2417      +/-   ##
==========================================
- Coverage   98.06%   98.03%   -0.03%     
==========================================
  Files         122      122              
  Lines       10687    10690       +3     
==========================================
  Hits        10480    10480              
- Misses        142      144       +2     
- Partials       65       66       +1
Impacted Files Coverage Δ
github/migrations_user.go 91.08% <40.00%> (-2.79%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@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 26, 2022
@gmlewis
Copy link
Collaborator

gmlewis commented Jul 26, 2022

@amitsaha - it looks like one of the examples needs to be updated. Please run go test ./... from the base directory of the repo to make sure all tests pass. Please see CONTRIBUTING.md for more details. Thanks.

@amitsaha
Copy link
Contributor Author

@amitsaha - it looks like one of the examples needs to be updated. Please run go test ./... from the base directory of the repo to make sure all tests pass. Please see CONTRIBUTING.md for more details. Thanks.

Thanks. I did that before I pushed my branch:

$ go test ./...
go: downloading golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be
go: downloading golang.org/x/net v0.0.0-20210226172049-e18ecbb05110
ok  	github.com/google/go-github/v45/github	0.840s
?   	github.com/google/go-github/v45/test/fields	[no test files]
?   	github.com/google/go-github/v45/test/integration	[no test files]

From the linting failures above I can see that there is an example needed to be updated, so I can see the failure when I run go test from within the example directory.

Not sure if I am doing something wrong?

Updating with the fix.

gmlewis
gmlewis approved these changes Jul 26, 2022
View reviewed changes
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, @amitsaha !
LGTM.

Awaiting second LGTM from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jul 26, 2022
@gmlewis gmlewis changed the title Fix #2416 Add ListOptions for listing user migrations Add ListOptions for listing user migrations Jul 26, 2022
@amitsaha
Copy link
Contributor Author

Thanks @gmlewis !

raynigon
raynigon approved these changes Aug 13, 2022
View reviewed changes
Copy link
Contributor

@raynigon raynigon left a comment

Choose a reason for hiding this comment

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

Can be merged, but it would be nice if the error case test gets added first.

github/migrations_user.go
Comment on lines +102 to +105
u, err := addOptions(u, opts)
if err != nil {
return nil, nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if this case got tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmlewis @raynigon I am actually not sure how I can test this path..The ListOptions{} takes in Page and PerPage. Is there an example you can point me to?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right... with the fixed URL, I'm not sure this can be tested, so let's leave it as-is. Thanks, @amitsaha .

@gmlewis gmlewis added waiting for reply and removed NeedsReview PR is awaiting a review before merging. labels Aug 19, 2022
@amitsaha amitsaha force-pushed the user_migrations_list_options branch from c23f90d to a518e88 Compare August 22, 2022 21:26
@amitsaha
Copy link
Contributor Author

@gmlewis Thanks! I know we had a new release just days back, is there any chance I can request you for a new release with this change?

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 22, 2022

@gmlewis Thanks! I know we had a new release just days back, is there any chance I can request you for a new release with this change?

Yes, I think the worst thing that would happen is I might annoy some people, but that's OK. I've done it before and survived. 😂

@amitsaha
Copy link
Contributor Author

@gmlewis Thanks! I know we had a new release just days back, is there any chance I can request you for a new release with this change?

Yes, I think the worst thing that would happen is I might annoy some people, but that's OK. I've done it before and survived. joy

Thanks a lot!

@gmlewis gmlewis merged commit dd22b73 into google:master Aug 22, 2022
@gmlewis gmlewis mentioned this pull request Aug 22, 2022
Merged
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 22, 2022

https://github.com/google/go-github/releases/tag/v47.0.0 now contains this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlewis gmlewis gmlewis approved these changes

@raynigon raynigon raynigon approved these changes

Assignees
No one assigned
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@amitsaha @gmlewis @raynigon