-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚨 Try these New Features:
|
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 |
2dcb9e8
to
adea978
Compare
adea978
to
2a32d65
Compare
2a32d65
to
d2545da
Compare
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. |
There was a problem hiding this 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).
Just for the reference: |
This PR proposes adding a new helper routine
github.Ptr
that can be used instead of existinggithub.Bool
,github.Int
,github.Int64
,github.String
.NOTE that this PR bumps the minimum required version of Go from 1.17 to 1.18.