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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexandear
Copy link
Contributor

@alexandear alexandear commented Nov 22, 2024

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.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.33%. Comparing base (2b8c7fa) to head (5ea1286).
Report is 175 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3355      +/-   ##
==========================================
- Coverage   97.72%   92.33%   -5.39%     
==========================================
  Files         153      176      +23     
  Lines       13390    15097    +1707     
==========================================
+ Hits        13085    13940     +855     
- Misses        215     1064     +849     
- Partials       90       93       +3     

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


🚨 Try these New Features:

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 22, 2024

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.

@gmlewis gmlewis changed the title feat: add github.Ptr to use instead of Bool,Int64,String feat: Deprecate and replace Bool,Int64,String with Ptr using generics Nov 22, 2024
@gmlewis gmlewis changed the title feat: Deprecate and replace Bool,Int64,String with Ptr using generics feat: Deprecate and replace Bool,Int,Int64,String with Ptr using generics Nov 22, 2024
github/actions_permissions_enterprise.go Outdated Show resolved Hide resolved
@gmlewis
Copy link
Collaborator

gmlewis commented Nov 22, 2024

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.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Nov 22, 2024
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, @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
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsReview PR is awaiting a review before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants