Skip to content

Add ListOptions for listing user migrations#2417

Merged
gmlewis merged 2 commits intogoogle:masterfrom
amitsaha:user_migrations_list_options
Aug 22, 2022
Merged

Add ListOptions for listing user migrations#2417
gmlewis merged 2 commits intogoogle:masterfrom
amitsaha:user_migrations_list_options

Conversation

@amitsaha
Copy link
Copy Markdown
Contributor

@amitsaha amitsaha commented Jul 26, 2022

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
Copy Markdown

codecov bot commented Jul 26, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

amitsaha 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.

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
Copy Markdown
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
Copy Markdown
Contributor Author

amitsaha commented Jul 27, 2022

Thanks @gmlewis !

raynigon
raynigon approved these changes Aug 13, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@raynigon raynigon Aug 13, 2022

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
Copy Markdown
Contributor Author

@amitsaha amitsaha Aug 22, 2022

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
Copy Markdown
Collaborator

@gmlewis gmlewis Aug 22, 2022

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
Copy Markdown
Contributor Author

amitsaha 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?

@gmlewis
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

amitsaha 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. 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
Copy Markdown
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

+1 more reviewer

@raynigon raynigon raynigon approved these changes

Reviewers whose approvals may not affect merge requirements

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.

3 participants

@amitsaha @gmlewis @raynigon