Prefer %v over %d,%s and add fmtpercentv custom linter

[gmlewis]

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.

[codecov]

Codecov Report

❌ Patch coverage is 88.43537% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.11%. Comparing base (6195c56) to head (69c428e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
example/commitpr/main.go	0.00%	6 Missing ⚠️
example/actionpermissions/main.go	0.00%	5 Missing ⚠️
example/verifyartifact/main.go	0.00%	3 Missing ⚠️
example/listenvironments/main.go	0.00%	1 Missing ⚠️
example/ratelimit/main.go	0.00%	1 Missing ⚠️
github/repos_contents.go	90.90%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3756   +/-   ##
=======================================
  Coverage   91.11%   91.11%           
=======================================
  Files         187      187           
  Lines       16702    16702           
=======================================
  Hits        15218    15218           
  Misses       1296     1296           
  Partials      188      188           

☔ View full report in Codecov by Sentry.
📢 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.

[gmlewis]

@stevehipwell - @alexandear - @zyfy29 - might you have time for a code review? Thank you!

[zyfy29]

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

[zyfy29]

+	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.

[gmlewis]

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?

[gmlewis]

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.

[zyfy29]

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

[zyfy29]

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.

[gmlewis]

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.

[gmlewis]

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.

[zyfy29]

Greet! LGTM

[gmlewis]

Thank you, @zyfy29!
Merging.