Skip to content

Update package constructors - #2904

Merged
gmlewis merged 7 commits into
google:masterfrom
WillAbides:clientopts3
Aug 31, 2023
Merged

Update package constructors#2904
gmlewis merged 7 commits into
google:masterfrom
WillAbides:clientopts3

Conversation

@WillAbides

Copy link
Copy Markdown
Contributor

Closes#2897

After experimenting with implementing #2897, I think this ends up with better usage for end users.

This deprecates github.NewTokenClient and github.NewEnterpriseClient and replaces them with Client.WithAuthToken and Client.WithEnterpriseURLs.

Now you can create an authenticated client for an enterprise server with:

client, err:=github.NewClient(nil).WithAuthToken(token).WithEnterpriseURLs(baseURL,uploadURL)

There is no need for users import github.com/x/oauth2 anymore.

I opted for this implementation over my WithOptions proposal the error returned from WithOptions means that most uses of go-github would have an unnecessary error check.

clienterr:=github.NewClient(nil).WithOptions(github.WithAuthToken(token))
iferr!=nil {
panic(err)
}

vs

client:=github.NewClient(nil).WithAuthToken(token)

@codecov

codecovBot commented Aug 28, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2904 (8a74e05) into master (c36edbd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2904 +/- ##
=======================================
Coverage 98.10% 98.11% =======================================
Files 142 142 Lines 12347 12385 +38 =======================================
+ Hits 12113 12151 +38 
Misses 159 159 Partials 75 75 
Files ChangedCoverage Δ
github/github.go98.10% <100.00%> (+0.10%)⬆️

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great, @WillAbides !
Just a few minor nits, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment threadgithub/github.go
Comment threadgithub/github.go
Comment threadgithub/github.go
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 28, 2023
WillAbidesand others added 2 commits August 29, 2023 09:02
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@WillAbides

Copy link
Copy Markdown
ContributorAuthor

@gmlewis Updated with your suggestions.

As an aside, is there a way to get a preview of what codecov is going to report before pushing? I looked for a way to run it locally, and it appears to require an API token and membership in the right org.

@gmlewis

Copy link
Copy Markdown
Collaborator

As an aside, is there a way to get a preview of what codecov is going to report before pushing? I looked for a way to run it locally, and it appears to require an API token and membership in the right org.

Not that I'm aware of.

Sometimes, to get an idea of what it might report, I run this locally first:

$ cat coverage-web.sh
#!/bin/bash -ex
go test -coverprofile=cover.out ./...
go tool cover -html cover.out

but it is obviously not the same.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome! Thank you, @WillAbides !
LGTM.

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

@WillAbides

Copy link
Copy Markdown
ContributorAuthor

I made a script to preview whether codecov will fail your PR: https://gist.github.com/WillAbides/e0bbffccb34d9922cecb2ab9d66defec

@gmlewis

Copy link
Copy Markdown
Collaborator

I made a script to preview whether codecov will fail your PR: https://gist.github.com/WillAbides/e0bbffccb34d9922cecb2ab9d66defec

Thank you, @WillAbides ! That's fantastic!

@sridharavinashsridharavinash left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks for the updates 💖 .

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 31, 2023
@gmlewisgmlewis changed the title Update constructorsUpdate package constructorsAug 31, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @sridharavinash !
Merging.

@gmlewis
gmlewis merged commit 5ab57e7 into google:masterAug 31, 2023
@WillAbides
WillAbides deleted the clientopts3 branch August 31, 2023 19:28
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
MichaelUrman added a commit to mutility/coverpkg that referenced this pull request Jan 10, 2024
google/go-github#2904 added support for bypassing github.com/x/oauth2. Use it.
MichaelUrman added a commit to mutility/coverpkg that referenced this pull request Jan 10, 2024
google/go-github#2904 added support for bypassing github.com/x/oauth2. Use it.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: client.WithOptions

3 participants

@WillAbides@gmlewis@sridharavinash