Skip to content

test: Replace &x variables with inline Ptr(value) calls#4289

Open
alexandear wants to merge 1 commit into
google:masterfrom
alexandear-org:test/use-ptr
Open

test: Replace &x variables with inline Ptr(value) calls#4289
alexandear wants to merge 1 commit into
google:masterfrom
alexandear-org:test/use-ptr

Conversation

@alexandear

Copy link
Copy Markdown
Contributor

This PR refactors tests. It replaces the pattern of declaring a temporary variable only to take its address:

name := "User One"
// ...
field: &name,

with an inline Ptr() call:

field: Ptr("User One"),

I created a custom golangci-lint linter to detect such cases, but it is not worth adding it because it increases the maintenance overhead.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jun 9, 2026

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

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

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.49%. Comparing base (946bd71) to head (b57ff59).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alexandear

Copy link
Copy Markdown
Contributor Author

I find this PR harder to read and reason about than what we had previously, and I do not see the benefit.

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.

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.

The main arguments for Ptr(value) inline:

  • Eliminates the "temp var just for address" anti-pattern - the variable name adds no semantic meaning
  • Consistent with how the rest of the codebase already uses Ptr()
  • Reduces noise: one line instead of two

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

OK, excellent points, @alexandear ... thank you!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @zyfy29 - @Not-Dhananjay-Mishra

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