Prefer `%v` over `%d`,`%s` and add `fmtpercentv` custom linter by gmlewis · Pull Request #3756 · google/go-github

Prefer `%v` over `%d`,`%s` and add `fmtpercentv` custom linter by gmlewis · Pull Request #3756 · google/go-github

This PR formalizes the preference of %v over the use of %s or %d in this repo by adding a custom linter that enforces this preference.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions. I'm not familiar with golangci custom linters, so please let me know if I'm wrong ☺️

Comment on lines +11 to +14

		_ = fmt.Sprintf("some/%d/url", 1) // want `use %v instead of %d`
		_ = fmt.Sprintf("some/%s/url", "yo") // want `use %v instead of %s`
		_ = fmt.Sprintf("some/%s/%d/url", "yo", 1) // want `use %v instead of %s and %d`
		_ = fmt.Sprintf("some/%d/%s/url", 1, "yo") // want `use %v instead of %s and %d`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+	fmt.Printf("some/%d/url", 1)               // want `use %v instead of %d`
+	fmt.Printf("some/%s/url", "yo")            // want `use %v instead of %s`

The linter currently only catches violations in assignment statements (e.g., url := fmt.Sprintf("some/%s", x)), but misses
standalone function calls like fmt.Printf("some/%s", x). I've reproduced it locally.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter currently only catches violations in assignment statements (e.g., url := fmt.Sprintf("some/%s", x)), but misses
standalone function calls like fmt.Printf("some/%s", x).

I suppose that it wouldn't hurt to be consistent throughout the entire repo.
I originally wrote it just to catch all the URL generators, but maybe it should do the whole repo.
Should I change the name of the linter to percentv instead of urlpercentv?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that it wouldn't hurt to be consistent throughout the entire repo.

I now remember why I didn't do this. It was because %s is perfectly valid when you have a []byte and I didn't want to have to flag all these usages with a // nolint... comment.

I believe the vast majority of these cases are in assignment statements, although I could be wrong. I could run some experiments.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, I didn't notice the linter name. It seems that all URL variable is created by a assignment statement. It's enough!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it could be avoided to use %s when converting a []byte to string (the only usage for the remaining %s). IMO since we have come to this point, it's better to lightly refactor it and ban all %s usage without using // nolint.... But it's a trade-off.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's a fair point. Let me investigate by removing the requirement for making an assignment and see what happens. First, though, I'll try to take care of unnecessary .String() calls... because that is so rare that I don't think we need a linter for it... or maybe golangci already has an option for it... 👀 no, apparently it does not... and I still don't think we need a linter for that.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

gmlewis changed the title ~~Prefer %v over %d,%s and add urlpercentv custom linter~~ Prefer %v over %d,%s and add fmtpercentv custom linter

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

OK, great idea, @zyfy29! I've renamed urlpercentv to fmtpercentv and generalized it to lint all call expression format strings, not just ones involving assignments. This only flagged 3 perfectly valid lines in the entire repo, so I've gone ahead and added the //nolint comments to those.

PTAL.

gmlewis deleted the add-url-percent-v-linter branch

October 6, 2025 01:40