Skip to content

feat: Deprecate and replace Bool,Int,Int64,String with Ptr using generics - #3355

Merged
gmlewis merged 3 commits into
google:masterfrom
alexandear-org:feat/add-github-ptr
Dec 11, 2024
Merged

feat: Deprecate and replace Bool,Int,Int64,String with Ptr using generics#3355
gmlewis merged 3 commits into
google:masterfrom
alexandear-org:feat/add-github-ptr

Conversation

@alexandear

@alexandearalexandear commented Nov 22, 2024

Copy link
Copy Markdown
Contributor

This PR proposes adding a new helper routine github.Ptr that can be used instead of existing github.Bool, github.Int, github.Int64, github.String.

NOTE that this PR bumps the minimum required version of Go from 1.17 to 1.18.

@codecov

codecovBot commented Nov 22, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.27%. Comparing base (2b8c7fa) to head (e9f8516).
Report is 193 commits behind head on master.

Files with missing linesPatch %Lines
example/actionpermissions/main.go0.00%3 Missing ⚠️
example/commitpr/main.go0.00%3 Missing ⚠️
example/newfilewithappauth/main.go0.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## master #3355 +/- ##
==========================================
- Coverage 97.72% 92.27% -5.45% 
==========================================
Files 153 176 +23 Lines 13390 15033 +1643 ==========================================
+ Hits 13085 13872 +787 - Misses 215 1068 +853 - Partials 90 93 +3 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmlewis

Copy link
Copy Markdown
Collaborator

I have mixed feelings about this change.

While I'm a huge fan of generics, that would change our base requirements of this repo from a minimum Go version of 1.17 to 1.18, if I'm not mistaken.

I have not heard any requests to maintain support for old versions of Go, and we have always officially tested the most recent release of Go and the prior minor version.

Let's see if this generates any discussion, pro or con.

Meanwhile, if we move forward with this, we should probably convert all the existing usages to Ptr.

@gmlewisgmlewis changed the title feat: add github.Ptr to use instead of Bool,Int64,Stringfeat: Deprecate and replace Bool,Int64,String with Ptr using genericsNov 22, 2024
@gmlewisgmlewis changed the title feat: Deprecate and replace Bool,Int64,String with Ptr using genericsfeat: Deprecate and replace Bool,Int,Int64,String with Ptr using genericsNov 22, 2024
Comment threadgithub/actions_permissions_enterprise.go
@gmlewis

Copy link
Copy Markdown
Collaborator

OK, I'm reviewing now. Please refrain from force-pushing if possible, as we always squash-and-merge in this repo so the commit history will be clean, and this will make reviewing changes in the PR much easier for the reviewer(s). Thanks.

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Nov 22, 2024

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

Thank you, @alexandear !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging (along with any discussion for or against this change, as usual, of course).

@alexandear

Copy link
Copy Markdown
ContributorAuthor

Just for the reference: xanzy/go-gitlab deprecated gitlab.String, gitlab.Bool, gitlab.Int in favor of gitlab.Ptr in xanzy/go-gitlab#1788 one year ago.

@stevehipwellstevehipwell 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 - But as a caveat I've not reviewed every line of code.

@gmlewis

Copy link
Copy Markdown
Collaborator

@alexandear - hearing no complaints about this PR, and having received one other LGTM (thank you, @stevehipwell !), let's go ahead and move forward with it.
If you could please merge the latest master into this PR and make sure it all still works, that would be greatly appreciated.
Thank you!

@alexandear

alexandear commented Dec 10, 2024

Copy link
Copy Markdown
ContributorAuthor

If you could please merge the latest master into this PR and make sure it all still works, that would be greatly appreciated.

Merged

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Dec 11, 2024
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @alexandear !
Merging.

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.

3 participants

@alexandear@gmlewis@stevehipwell