Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Oct 31, 2025

https://github.com/golangci/golangci-lint/releases/tag/v2.6.0

  • modernize linter is enabled, this is the same as gopls modernize
  • perfsprint linter is disabled because it conflicts with modernize (maybe there is a middle ground)
  • gocritic deprecatedComment is disabled as it conflicts with go-swagger

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 31, 2025
@github-actions github-actions bot added topic/code-linting modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/internal labels Oct 31, 2025
@silverwind
Copy link
Member Author

silverwind commented Oct 31, 2025

I think I will revert these string builder changes and disable the offending rule, it seems useless.

@silverwind
Copy link
Member Author

silverwind commented Oct 31, 2025

The stringsbuilder rule is from modernize actually:

https://github.com/golang/tools/blob/ed067b1f257070c651750130ce5081e4afc1915f/gopls/doc/analyzers.md#stringsbuilder-replace--with-stringsbuilder

It says "This avoids quadratic memory allocation and improves performance." so maybe it is worth it.

I will fix those var names.

@silverwind silverwind marked this pull request as draft October 31, 2025 09:39
@silverwind silverwind marked this pull request as ready for review November 1, 2025 10:46
@silverwind
Copy link
Member Author

I manually cleaned up the stringsbuilder cases, should be ready.

@wxiaoguang
Copy link
Contributor

I really dislike unnecessary lint rules.

And I have strong objection for this Sprintf change, it doesn't improve readability.

@silverwind
Copy link
Member Author

silverwind commented Nov 3, 2025

I think we could disable stringsbuilder in modernize if it's deemed useless. I guess the "quadratic" aspect of it would only be noticed with a sufficient iteration count. Also I think it's inherently golang's fault if string concatenation is O(n^2) instead of O(n) or O(1) complexity.

@github-actions github-actions bot removed the modifies/api This PR adds API routes or modifies them label Nov 3, 2025
@silverwind
Copy link
Member Author

I've disabled modernize.stringsbuilder and perfsprint.concat-loop so string concatenation in loops is allowed again and the diff is much reduced.

@silverwind
Copy link
Member Author

The previous autofixes with the ugly var names were from perfsprint. When I try modernize's autofix, it does at least leave the var names intact, so if I enable stringsbuilder it produces a diff like this:

diff --git i/modules/git/foreachref/format.go w/modules/git/foreachref/format.go
index d9573a55d6..d2f9998fe8 100644
--- i/modules/git/foreachref/format.go
+++ w/modules/git/foreachref/format.go
@@ -74,10 +74,10 @@ func (f Format) Parser(r io.Reader) *Parser {

 // hexEscaped produces hex-escpaed characters from a string. For example, "\n\0"
 // would turn into "%0a%00".
 func (f Format) hexEscaped(delim []byte) string {
-	escaped := ""
+	var escaped strings.Builder
 	for i := range delim {
-		escaped += "%" + hex.EncodeToString([]byte{delim[i]})
+		escaped.WriteString("%" + hex.EncodeToString([]byte{delim[i]}))
 	}
-	return escaped
+	return escaped.String()
 }

Would that be an acceptable fix?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 3, 2025
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Nov 3, 2025
@silverwind
Copy link
Member Author

silverwind commented Nov 3, 2025

I've re-enabled the modernize rule and ran make lint-go-fix, it looks acceptable imho, if not, revert the last commit.

@github-actions github-actions bot removed the modifies/api This PR adds API routes or modifies them label Nov 3, 2025
@silverwind
Copy link
Member Author

Actually, I've reverted, we can discuss the merits of stringsbuilder another time, let's merge this first.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 4, 2025
@techknowlogick techknowlogick enabled auto-merge (squash) November 4, 2025 02:33
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 4, 2025
@techknowlogick techknowlogick merged commit 850012b into go-gitea:main Nov 4, 2025
25 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Nov 4, 2025
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 4, 2025
@silverwind silverwind deleted the golangci-lint@v2.6.0 branch November 4, 2025 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/internal topic/code-linting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants