Skip to content

Comments

fix perfsprint lints#1755

Closed
jordanstephens wants to merge 1 commit intomainfrom
jordan/fix-perfsprint-lints
Closed

fix perfsprint lints#1755
jordanstephens wants to merge 1 commit intomainfrom
jordan/fix-perfsprint-lints

Conversation

@jordanstephens
Copy link
Member

@jordanstephens jordanstephens commented Jan 7, 2026

Just started seeing these warnings in my lint output:

make[1]: Entering directory '/Users/jordan/wk/defang/src'
src/pkg/agent/plugins/compat_oai/generate.go:249:3: concat-loop: string concatenation in a loop (perfsprint)
		content += part.Text
		^
src/pkg/cli/client/byoc/gcp/byoc.go:851:3: concat-loop: string concatenation in a loop (perfsprint)
		result += logTimestamp + " " + msg + "\n"
		^
src/pkg/cli/client/byoc/parse.go:32:4: concat-loop: string concatenation in a loop (perfsprint)
			pending += " " + strconv.Quote(p)
			^
3 issues:
* perfsprint: 3
Run 'make lint-fix' to try to fix the linting errors
make[1]: *** [Makefile:172: lint] Error 1
make[1]: Leaving directory '/Users/jordan/wk/defang/src'
make: *** [Makefile:10: pre-commit] Error 2

Seems like it's trying to encourage us to use string builders.

Description

Linked Issues

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • Refactor
    • Optimized internal string handling operations for improved performance across multiple components. These changes maintain existing functionality while reducing memory allocation overhead during string construction.

✏️ Tip: You can customize this high-level summary in your review settings.

concat-loop: string concatenation in a loop
@jordanstephens jordanstephens requested review from edwardrf and removed request for lionello January 7, 2026 18:49
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Three functions optimize string handling: replacing direct string concatenation with byte slice accumulation or strings.Builder to reduce memory allocations. No functional behavior, error handling, or API changes; output remains identical.

Changes

Cohort / File(s) Summary
String Accumulation Performance Optimizations
src/pkg/agent/plugins/compat_oai/generate.go, src/pkg/cli/client/byoc/gcp/byoc.go, src/pkg/cli/client/byoc/parse.go
Refactored three functions to replace inefficient string concatenation patterns with byte slice accumulation (ModelGenerator.concatenateContent) or strings.Builder (LogEntriesToString, PulumiState.String()). Observable output unchanged; avoids repeated string reallocations and improves performance.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops through code so neat,
Where strings once sprawled and grew concrete,
Now builders gather, bytes align—
More efficient paths that shine!
🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix perfsprint lints' directly and specifically describes the main change: addressing perfsprint linter warnings by replacing string concatenation with builders.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1b948d3 and bf0d17d.

📒 Files selected for processing (3)
  • src/pkg/agent/plugins/compat_oai/generate.go
  • src/pkg/cli/client/byoc/gcp/byoc.go
  • src/pkg/cli/client/byoc/parse.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.

Applied to files:

  • src/pkg/cli/client/byoc/parse.go
  • src/pkg/agent/plugins/compat_oai/generate.go
  • src/pkg/cli/client/byoc/gcp/byoc.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (go)
  • GitHub Check: go-test
🔇 Additional comments (3)
src/pkg/agent/plugins/compat_oai/generate.go (1)

246-252: LGTM!

The []byte accumulator correctly addresses the perfsprint lint. Using append(b, part.Text...) efficiently accumulates string bytes without intermediate allocations per iteration.

src/pkg/cli/client/byoc/parse.go (1)

30-37: LGTM!

The strings.Builder refactor correctly addresses the perfsprint lint while preserving the output format. This is the idiomatic Go approach for efficient string construction in loops.

src/pkg/cli/client/byoc/gcp/byoc.go (1)

844-857: LGTM!

The strings.Builder refactor correctly addresses the perfsprint lint. The separate WriteString calls for timestamp, space, message, and newline preserve the original output format while avoiding allocations from repeated string concatenation.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lionello
Copy link
Member

lionello commented Jan 7, 2026

Was fixed in #1751

@lionello lionello closed this Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants