CBG-5969 gopls: use fmt.Errorf vs errors.New - #8499
Conversation
|
Droid finished @torcolvin's task —— View job |
There was a problem hiding this comment.
🟡 Not ready to approve
Enabling S1028 will fail lint due to remaining errors.New(fmt.Sprintf(...)) violations (e.g., in base/logger_file.go), and one updated error message string in base/bucket.go is currently malformed/confusing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR applies Go linter-driven cleanups across Sync Gateway’s codebase by simplifying error construction (using fmt.Errorf) and simplifying a regular expression literal, and then updates the strict golangci configuration to stop excluding the corresponding staticcheck rule.
Changes:
- Replace
errors.New(fmt.Sprintf(...))withfmt.Errorf(...)in several call sites. - Convert the HTTP range regex to a raw string literal to avoid double-escaping.
- Remove the
.golangci-strict.ymlexclusion for staticcheckS1028.
File summaries
| File | Description |
|---|---|
| rest/server_context.go | Simplifies an “unknown event handler type” error to use fmt.Errorf. |
| rest/handler.go | Uses a raw string literal for the byte-range regex to match staticcheck guidance. |
| base/leaky_datastore.go | Simplifies artificial leaky-bucket error construction with fmt.Errorf. |
| base/bucket.go | Simplifies error construction when the mgmt request returns a non-200 status. |
| .golangci-strict.yml | Enables S1028 by removing its exclusion from the strict linter config. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
There was a problem hiding this comment.
Enabling staticcheck S1028 is a good cleanup, but there are still S1028 violations elsewhere in the repo that will likely break strict lint. There is also a small, user-visible error-string typo in base/bucket.go that is easy to correct while touching this area.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| WarnfCtx(ctx, "403 Forbidden attempting to access %s. Bucket user must have Bucket Full Access and Bucket Admin roles to retrieve metadata purge interval.", UD(uri)) | ||
| } else if statusCode != http.StatusOK { | ||
| return 0, errors.New(fmt.Sprintf("failed with status code, %d, statusCode", statusCode)) | ||
| return 0, fmt.Errorf("failed to retrieve purge interval from %s: status code %d", UD(uri), statusCode) |
There was a problem hiding this comment.
fmt.Errorf won't redact but I'm not convinced it needs to be part of the error anyway - the caller knows what bucket it is which is probably the only relevant part of URI
There was a problem hiding this comment.
I think this is pretty unlikely to fail but I added base.RedactErrorf if it shows up in logging and otherwise it gets returned to a user.
CBG-5969 gopls: use fmt.Errorf vs errors.New and use string literal for regex.