test: Replace &x variables with inline Ptr(value) calls#4289
Conversation
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
I find this PR harder to read and reason about than what we had previously, and I do not see the benefit. I personally would prefer to close this PR but I'm willing to listen to any arguments as to why we should proceed with this.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4289 +/- ##
=======================================
Coverage 97.49% 97.49%
=======================================
Files 192 192
Lines 19256 19256
=======================================
Hits 18774 18774
Misses 267 267
Partials 215 215 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Previously in this discussion we had slightly different code, where the variable was used twice. lastActiveOn := &Timestamp{time.Date(2023, 4, 26, 15, 23, 37, 0, time.UTC)}
// ...
LastActiveOn: Ptr(lastActiveOn)
// ...
LastActiveOn: Ptr(lastActiveOn)But in this PR variables are used only once.
The main arguments for
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
OK, excellent points, @alexandear ... thank you!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
LGTM
@alexandear I assume that the intention here is to replace Ptr with new when we upgrade to 1.26?
Yes, it is. The command |
|
Thank you, @alexandear, @Not-Dhananjay-Mishra, and @stevehipwell! |
4 participants
This PR refactors tests. It replaces the pattern of declaring a temporary variable only to take its address:
with an inline
Ptr()call:I created a custom golangci-lint linter to detect such cases, but it is not worth adding it because it increases the maintenance overhead.